Gen typed params and latest resources: autogen java spec a5eee30#683
Conversation
mickjermsurawong-stripe
changed the title
remi-stripe
left a comment
remi-stripe
left a comment
There was a problem hiding this comment.
So I started going through it but I'll be honest I am not sure how I can review a PR like this. Reviewing every single changes would take numerous hours (and I'd miss things quickly unless I check the API for every single thing.
@mickjermsurawong-stripe do you have any advice on how to approach this?
I'm also realizing how complex PRs will be to review since you don't have to look at one property anymore, you have to look at all the changes across related builder classes, internal properties and such. This part, I don't feel comfortable owning myself.
| * dictionary containing a user's bank account details. | ||
| */ | ||
| @SerializedName("bank_account") | ||
| Object bankAccount; |
There was a problem hiding this comment.
Why is this one an Object when Company is a class. I'm assuming it's because it has different shapes but what does the code using it looks like?
Also, I'm realizing that bank_account should not be supported at all. Only external_account should be supported. bank_account was removed in 2015 in theory: https://stripe.com/docs/upgrades#2015-10-01
There was a problem hiding this comment.
This is removed and now we have externalAccount String token instead.
The documentation is still wrong though. I have a PR for gated docs that doesn't change that in API refs (saying that it's still a hash)
|
|
||
| /** Specifies which fields in the response should be expanded. */ | ||
| @SerializedName("expand") | ||
| List<String> expand; |
There was a problem hiding this comment.
Can you confirm whether we're always going to have this property on all params even if nothing is expandable?
There was a problem hiding this comment.
ahh thank you remi! no. we should not expect this. I will have to fix this in OpenAPI spec.
There was a problem hiding this comment.
It is likely easier to just have it always. Though I'm fine with not having it.
There was a problem hiding this comment.
Yup we currently have it as default except request with DELETE endpoint
| * have some of its requested capabilities be active and some be inactive. | ||
| */ | ||
| @SerializedName("requested_capabilities") | ||
| List<RequestedCapability> requestedCapabilities; |
There was a problem hiding this comment.
I did not know we were going with enums in params. Is that expected? While I'm fine in principle, I would say that enum on resources are higher priority than on params (personal opinion). This is a bad example as it's not returned in the API but I've confirmed that disabled_reason for example on the Account is not an enum.
There was a problem hiding this comment.
Yup this is expected. Enum on resource introduced now will cause too much incompatibility with 7.x. But here since we start from zero, we introduce Enum as well.
There was a problem hiding this comment.
Ack okay. Does Java let you pass a string there easily instead of an enum value? There are a lot of properties that have gated enum values
There was a problem hiding this comment.
We have .toMap exposed to users so they can always add gated/beta content the Map, and use the old way of calling API
|
|
||
| /** Whether information has been collected from the company's directors. */ | ||
| @SerializedName("directors_provided") | ||
| Boolean directorsProvided; |
There was a problem hiding this comment.
Random but wanted to make sure we won't have the same issue as we had in go for years where this sends false if it's not initialized. We had to move to pointers and that was a lot of work.
| * Either a token, like the ones returned by [Stripe.js](https://stripe.com/docs/stripe.js), or | ||
| * a dictionary containing a user's bank account details. | ||
| */ | ||
| public Builder setBankAccount(Map<String, Object> bankAccount) { |
There was a problem hiding this comment.
@remi-stripe when a field is polymorphic, the builder will have explicit setters for each type. So using the builder, one will have this effect of setBankAccount showing explicit options for all the possible types.
The getter of the built object though will have return type Object that users have to cast, but that's not the expected use case.
There was a problem hiding this comment.
Ack okay. I got confused because I expected the String or Class of params for bank accounts like @ob-stripe did in .net with AnyOf
|
|
||
| @Getter | ||
| public class CardUpdateOnCustomerParams extends ApiRequestParams { | ||
| /** The name of the person or business that owns the bank account. */ |
There was a problem hiding this comment.
Flagging this parameter doc is incorrect.
There was a problem hiding this comment.
Updated. Card params now have only card-related values
| import lombok.Getter; | ||
|
|
||
| @Getter | ||
| public class CardUpdateOnCustomerParams extends ApiRequestParams { |
There was a problem hiding this comment.
So I was looking for this and got quickly confused about this naming. It seems hard to "discover"
There was a problem hiding this comment.
I suspect the pattern of usage is from the method call where they look at method signature, instead of going through our list of params.
Do you think naming like CustomerCardUpdate and AccountCardUpdate would be better? We can do CustomerCardUpdate and PayoutCardUpdate but that will be more confusing.
|
|
||
| /** The type of entity that holds the account. This can be either `individual` or `company`. */ | ||
| @SerializedName("account_holder_type") | ||
| AccountHolderType accountHolderType; |
There was a problem hiding this comment.
this and above only exists for Bank Account and not Card
There was a problem hiding this comment.
Addressed
| String coupon; | ||
|
|
||
| @SerializedName("customer") | ||
| String customer; |
There was a problem hiding this comment.
This should not exist on customer creation as far as I can tell.
There was a problem hiding this comment.
Addressed
|
|
||
| /** A custom ID to use for the customer. */ | ||
| @SerializedName("id") | ||
| String id; |
There was a problem hiding this comment.
This should be in the library. It's been deprecated/undocumented for years.
There was a problem hiding this comment.
It now goes away together with customer
|
|
||
| private Map<String, String> metadata; | ||
|
|
||
| private Object shipping; |
There was a problem hiding this comment.
This is strange to see. I looked at the setters and it's because of unsetting the entire hash so I kind of understand but I wonder if there's a better solution.
There was a problem hiding this comment.
Do you mean the EMPTY enum for setShipping?
There was a problem hiding this comment.
Yes. This feels quite weird and I noticed it in other places. I guess we might not have a choice. Just looks quite hacky/wrong to me so wanted to raise.
There was a problem hiding this comment.
We actually discussed this during the implementation phase, and agreed on EMPTY
https://paper.dropbox.com/doc/Polymorphic-params-in-stripe-java--AZOgnte1Z7stWBn8JIdFsIe4Ag-amkaRe4C2QWqjVWcONtWx
| return this; | ||
| } | ||
|
|
||
| public Builder setSource(Map<String, Object> source) { |
There was a problem hiding this comment.
I don't think we should support a map here. In theory, we should go all the way and support passing the correct list of params that are valid for the source hash like card's number, cvc, etc.
If we do, there's a big documentation risk as we need to make it clear no one should use this.
There was a problem hiding this comment.
Ah we don't have this encoded in OpenAPI spec for now.. Let me see if I can fix it upstream..
There was a problem hiding this comment.
Yeah this is one of those where I'm ambivalent. I want us to support it properly. But then more and more merchants will send raw PANs because it looks easy in the library. Not sure what the right product decision is here. Maybe the hash is easier because less people would find it.
There was a problem hiding this comment.
As discussed, it's a now a String
remi-stripe
left a comment
remi-stripe
left a comment
There was a problem hiding this comment.
I'm selecting a few params I know well to list things that I find
| * month for subsequent invoices. | ||
| */ | ||
| @SerializedName("billing_cycle_anchor") | ||
| String billingCycleAnchor; |
There was a problem hiding this comment.
This should be long right? since it's a unix timestamp
There was a problem hiding this comment.
@remi-stripe, I realized I haven't fixed this type inconsistencies
| * trial immediately. | ||
| */ | ||
| @SerializedName("trial_end") | ||
| String trialEnd; |
There was a problem hiding this comment.
This should be a string for now or a long for unix timestamp. Would it be cleaner to handle this the right way?
There was a problem hiding this comment.
here too
| */ | ||
| @SerializedName("metadata") | ||
| Map<String, String> metadata; | ||
|
|
There was a problem hiding this comment.
We're missing plan though I guess it's on purpose because we consider it deprecated and we're fine forcing merchants to not support it? (it sounds okay especially as there's always the escape hatch anyway, just asking to confirm)
There was a problem hiding this comment.
@remi-stripe I still haven't addressed this "missing" plan field and also Customer create too.
Are we okay with releasing without first?
| Account account; | ||
|
|
||
| @SerializedName("amount") | ||
| Long amount; |
There was a problem hiding this comment.
This should not be in this API. Please make sure to remove/document in pay-server so that it can't leak again. This is a really old and sensitive parameter that lets you do card testing really easily :(
There was a problem hiding this comment.
This and the token fields below are addressed
|
Thanks @remi-stripe! On tokens there those internal fields, I will have to undoc them from public spec. On the deprecated fields that are introduced, like So I think we likely want to ignore this forever change in SDK spec, and follow the latest params instead similar to that in API refs instead. That's a little tricky because the API refs also does lots of custom handling for these fields.. |
mickjermsurawong-stripe
force-pushed
the
autogen/8.0-beta
branch
from
5340c29 to
18ec265
Compare
mickjermsurawong-stripe
force-pushed
the
mickjermsurawong/sdk-autogen-java-spec-a5eee30
branch
from
30e0b44 to
e6f442c
Compare
mickjermsurawong-stripe
changed the base branch from
autogen/8.0-beta
to
mickjermsurawong/sdk-autogen-java-non-master-spec-e27a05e_4
mickjermsurawong-stripe
force-pushed
the
mickjermsurawong/sdk-autogen-java-spec-a5eee30
branch
2 times, most recently
from
541e99c to
5a493a2
Compare
|
@remi-stripe Could you help to look at a few more critical params please? |
|
ptal @remi-stripe |
remi-stripe
left a comment
remi-stripe
left a comment
There was a problem hiding this comment.
looked at a few random ones and added comments
| */ | ||
| @SerializedName("metadata") | ||
| Map<String, String> metadata; | ||
|
|
There was a problem hiding this comment.
You're missing plan here. Billing will likely say they don't want to support this and would like to deprecate it but it's actively used. Technically you can add it via the map so it's not a deal breaker but I wanted to flag, same for quantity and tax_percent and other fields. See https://github.com/stripe/stripe-dotnet/blob/master/src/Stripe.net/Services/Customers/CustomerCreateOptions.cs#L31
I'm flagging this because this will be really common. An eng team will want to undocument something but removing it from stripe-java will be a major version. And we tend to not do that in go and .net. Doing this is a "waste" of major versions and we tend to deprecate what's actively removed on an API version instead.
| String externalAccount; | ||
|
|
||
| @SerializedName("from_recipient") | ||
| String fromRecipient; |
There was a problem hiding this comment.
This has been undocumented for a while, why is it appearing here when others undocumented things don't appear?
There was a problem hiding this comment.
addressed in 760b5b5#diff-a9f0113cf10a4a96bc61507995cc5a57R62
| return this; | ||
| } | ||
|
|
||
| public Builder setCard(EncryptedCard card) { |
There was a problem hiding this comment.
This is secret and gated and I don't think it should be here
There was a problem hiding this comment.
addressed
| return this; | ||
| } | ||
|
|
||
| public Builder setCard(ApplePay card) { |
There was a problem hiding this comment.
I don't think this should be in stripe-java. If it's what I think it's about creating a card token from an Apple Pay PKToken and should only be in stripe-ios
There was a problem hiding this comment.
addressed
| return this; | ||
| } | ||
|
|
||
| public Builder setCard(CreditCard card) { |
There was a problem hiding this comment.
CreditCard is a risky name for a class like this. It implies Debit cards would not work
There was a problem hiding this comment.
Addressed: annotated in the new PR
| String customer; | ||
|
|
||
| @SerializedName("default_for_currency") | ||
| Boolean defaultForCurrency; |
There was a problem hiding this comment.
I don't think this is a real thing on token creation so I don't get why it's here
There was a problem hiding this comment.
Addressed
| String number; | ||
|
|
||
| @SerializedName("number_looks_valid") | ||
| Boolean numberLooksValid; |
There was a problem hiding this comment.
same, this looks like really leaking something
There was a problem hiding this comment.
Addressed
| */ | ||
| public Builder putMetadata(String key, String value) { | ||
| if (this.metadata == null) { | ||
| t |
There was a problem hiding this comment.
This should not be here, there's no 3DS class on token creation and if there is it's some weird internal thing I would say.
There was a problem hiding this comment.
Removed 3d secured
|
|
||
| public enum NotificationMethod implements ApiRequestParams.Enum { | ||
| @SerializedName("deprecated_none") | ||
| DEPRECATED_NONE("deprecated_none"), |
There was a problem hiding this comment.
Can you clarify what this one even is?
There was a problem hiding this comment.
This goes away when we decide to undoc mandate
| DEPRECATED_NONE("deprecated_none"), | ||
|
|
||
| @SerializedName("email") | ||
| EMAIL("email"), |
There was a problem hiding this comment.
What is that EMAIL() thing
There was a problem hiding this comment.
Same because of undoc mandate
mickjermsurawong-stripe
changed the base branch from
mickjermsurawong/sdk-autogen-java-non-master-spec-e27a05e_4
to
autogen-params
mickjermsurawong-stripe
changed the base branch from
autogen-params
to
mickjermsurawong/sdk-autogen-java-non-master-spec-e27a05e_4
mickjermsurawong-stripe
force-pushed
the
mickjermsurawong/sdk-autogen-java-non-master-spec-e27a05e_4
branch
from
89c74af to
0278766
Compare
mickjermsurawong-stripe
force-pushed
the
mickjermsurawong/sdk-autogen-java-spec-a5eee30
branch
from
5a493a2 to
0278766
Compare
mickjermsurawong-stripe
deleted the
mickjermsurawong/sdk-autogen-java-spec-a5eee30
branch
mickjermsurawong-stripe
restored the
mickjermsurawong/sdk-autogen-java-spec-a5eee30
branch
2 participants
This is the latest updates that introduced typed params and new resources.
Jar from this is also used to run codes to verify api sample refs are up-to-date
cc @stripe/api-libraries