Introduce version pinning for autogen #651

mickjermsurawong-stripe · 2019-01-11T23:45:14Z

This should not be merged to master but to mickjermsurawong/autogen-pre-master branch, but i'm basing here on master to show compatibility and to pass master tests first.
The only complication is in EphemeralKey that does not use the static version. (This class is together with File, Event, are not autogenerated).
- Introduces stripeVersionOnBehalfOf for request options. This is different from both .NET and Go. where it takes version as the method params. I think that it would be better continue the pattern of header-related values to request option, and not in the parameters map.
  r? @brandur-stripe
  cc @stripe/api-libraries

mickjermsurawong-stripe · 2019-01-11T23:47:26Z

src/main/java/com/stripe/net/RequestOptions.java

+   * Stripe version when made on behalf of others. This can be used when the returned response
+   * will not be deserialized into the current classes pinned to {@link Stripe#VERSION}.
+   */
+  private final String stripeVersionOnBehalfOf;


Generalize the stripe version of mobile client required by ephemeral key to stripeVersionOnBehalfOf.
Putting in request option here avoid considering stripe version here has method params.

mickjermsurawong-stripe · 2019-01-11T23:48:00Z

src/main/java/com/stripe/net/RequestOptions.java

+        : 0);
    result = 31 * result + (idempotencyKey != null ? idempotencyKey.hashCode() : 0);
-    result = 31 * result + readTimeout;
+    result = 31 * result + (stripeAccount != null ? stripeAccount.hashCode() : 0);


stripeAccount is added, although not related to this PR. I think it was unintentionally left out

+1. That's the thing with these manual implementations — it's really easy to forget things.

We could probably replace these manual implementations with Lombok's @EqualsAndHashCode.

whoops, sorry i missed this and merged to my branch without it. Will update in subsequent PR

brandur-stripe

A few minor comments, but LGTM.

@ob-stripe You may want to take a look as well.

brandur-stripe · 2019-01-14T18:56:32Z

src/main/java/com/stripe/Stripe.java

  public static final String CONNECT_API_BASE = "https://connect.stripe.com";
  public static final String UPLOAD_API_BASE = "https://files.stripe.com";
  public static final String VERSION = "7.14.0";
+  public static final String API_VERSION = "2018-11-08";


Can you put API_VERSION in here alphabetically?

brandur-stripe · 2019-01-14T18:59:07Z

src/main/java/com/stripe/net/RequestOptions.java

    private String apiKey;
    private String clientId;
-    private String stripeVersion;
+    private String stripeVersionOnBehalfOf;


Similarly, alphabetize this (basically any list of things that looks like someone's put in the effort to alphabetize initially).

brandur-stripe · 2019-01-14T19:04:05Z

src/main/java/com/stripe/net/RequestOptions.java

+     * @param stripeVersionOnBehalfOf stripe version of the client to make request on behalf of
+     * @return option builder
+     */
+    public RequestOptionsBuilder setStripeVersionOnBehalfOf(String stripeVersionOnBehalfOf) {


Not too sure about the naming here — can we just call it setStripeVersion? We can specify in the comments that it'll override the built-in version.

@brandur, I'm wondering if i should make the name different here. I'm afraid that merchants who are using pre-pinning version still have setStripeVersion invocations, and once they migrate to pinned-version will not realize the semantic difference here.
Making it explicitly different here, and remove the old method will at least ensure that they set up the integration correctly.

Would naming like overrideStripeVersion would be a better alternative?

Oh oops — I didn't read carefully enough and didn't realize that setStripeVersion could already be called for RequestOptions before.

Yeah, I was actually going to suggest using "override" in the name as an alternative before, so totally okay with that. I'd suggest stripeVersionOverride instead of overrideStripeVersion though since it sorts more nicely.

mickjermsurawong-stripe · 2019-01-15T00:41:27Z

src/main/java/com/stripe/net/RequestOptions.java

      return false;
    }
-    if (stripeVersion != null ? !stripeVersion.equals(that.stripeVersion)
-        : that.stripeVersion != null) {


Reordering to match the alphabetically sorted instance variables, plus adding stripeAccount

mickjermsurawong-stripe · 2019-01-15T00:52:52Z

thank you @brandur! I cleaned up variable alphabetical order, and renamed the the stripeOnBehalfOf to stripeVersionOverride
ptal @ob-stripe

ob-stripe · 2019-01-15T09:48:25Z

LGTM!

mickjermsurawong-stripe · 2019-01-15T17:14:38Z

thank you both! will rebase this on my pre-autogen branch

mickjermsurawong-stripe · 2019-01-15T17:16:22Z

self-approving after rebase on autogen branch
LGTM

…rawong/version-pin-java make alphabetically order rename to override instead of onBehalf of

introduce pinning

a774909

mickjermsurawong-stripe commented Jan 11, 2019

View reviewed changes

stripe-ci assigned brandur-stripe Jan 11, 2019

add doc and remove onbehalf of

0b6e715

brandur-stripe reviewed Jan 14, 2019

View reviewed changes

make alphabetically order

1cdec93

mickjermsurawong-stripe force-pushed the mickjermsurawong/version-pin-java branch 2 times, most recently from 9a34152 to 5c154c9 Compare January 15, 2019 00:38

mickjermsurawong-stripe commented Jan 15, 2019

View reviewed changes

rename to override instead of onBehalf of

4274a86

mickjermsurawong-stripe force-pushed the mickjermsurawong/version-pin-java branch from 5c154c9 to 4274a86 Compare January 15, 2019 00:48

mickjermsurawong-stripe changed the base branch from master to mickjermsurawong/autogen-pre-master January 15, 2019 17:15

mickjermsurawong-stripe changed the title ~~[wip] Introduce version pinning~~ Introduce version pinning for autogen Jan 15, 2019

mickjermsurawong-stripe merged commit c4800ed into mickjermsurawong/autogen-pre-master Jan 15, 2019

mickjermsurawong-stripe mentioned this pull request Jan 16, 2019

Best-attempt event read on version pinning #654

Closed

mickjermsurawong-stripe added a commit that referenced this pull request Jan 21, 2019

introduce version pinning. Rebase from PR #651 from stripe/mickjermsu…

bbe00ba

…rawong/version-pin-java make alphabetically order rename to override instead of onBehalf of

mickjermsurawong-stripe added a commit that referenced this pull request Jan 22, 2019

introduce version pinning. Rebase from PR #651 from stripe/mickjermsu…

573bfa2

…rawong/version-pin-java make alphabetically order rename to override instead of onBehalf of

mickjermsurawong-stripe added a commit that referenced this pull request Jan 25, 2019

introduce version pinning. Rebase from PR #651 from stripe/mickjermsu…

0ef7044

…rawong/version-pin-java make alphabetically order rename to override instead of onBehalf of

mickjermsurawong-stripe added a commit that referenced this pull request Jan 26, 2019

introduce version pinning. Rebase from PR #651 from stripe/mickjermsu…

fafdf0b

…rawong/version-pin-java make alphabetically order rename to override instead of onBehalf of

ob-stripe deleted the mickjermsurawong/version-pin-java branch April 10, 2019 16:35

Introduce version pinning for autogen #651

Introduce version pinning for autogen #651

Uh oh!

Conversation

mickjermsurawong-stripe commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandur-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mickjermsurawong-stripe commented Jan 15, 2019

Uh oh!

ob-stripe commented Jan 15, 2019

Uh oh!

mickjermsurawong-stripe commented Jan 15, 2019

Uh oh!

mickjermsurawong-stripe commented Jan 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mickjermsurawong-stripe commented Jan 11, 2019 •

edited

Loading