stripe / openapi

An OpenAPI specification for the Stripe API.
MIT License
394 stars 123 forks source link

Request: Include max length for string fields #26

Closed jleclanche closed 6 years ago

jleclanche commented 6 years ago

Can the max length of string fields be included in the spec somehow?

We need it in dj-stripe to be able to store whatever the API throws at us. Creating arbitrary text fields for everything is inconvenient, and not compatible with some databases (eg. some of them need to be unique and mysql doesn't like unique indices on text fields).

jleclanche commented 6 years ago

Similarly if there's any other declarative validation methods that can be included in there such as min/max for ints etc that'd be fantastic...

brandur commented 6 years ago

Yes, I think this should be possible, but I need some more time to get them in — max string validation on the backend has always been a bit of a mess and I still need to through and unwind a few more problems to get to something cohesive enough to reflect into OpenAPI.

It'd be great to get other validations in here too, but that might be more of an incremental process. If there some in particular that are important, it'd probably be better to request them one by one.

jleclanche commented 6 years ago

Great! Sounds good.

If there some in particular that are important, it'd probably be better to request them one by one.

The oddballs don't necessarily need to be in the openapi spec, but documented at least. I'm thinking cases such as dates that can't be more than n years in the future etc.

jleclanche commented 6 years ago

Since I need the maxlengths for dj-stripe I just use the dashboard to create test objects. It does appear there's multiple layers of max length validations.

I got trolled by the URL field on Product objects:

image

image

image

:thinking:

brandur-stripe commented 6 years ago

5,000 is the general default maximum length, but there's quite a bit of variation when it comes to various fields.

Alright, I'm going to push through a change to add maximum lengths to most strings, but just a word of warning in advance: (1) for now we're not going to guarantee maximum string lengths as part of our API stability because there's at least a number of them that were not happy with (ideally, most of our strings would have a shorter maximum length), and (2) I'm not going to guarantee the absolute accuracy of these either. Most of them will be right, but there's a few places where it's not particularly easy to extract a maximum length, and those cases will be best effort for now.

jleclanche commented 6 years ago

@brandur-stripe Sounds great. Yes, fully understood on guarantees. The main one that causes me trouble is enum fields. I don't want to set their max length to the maximum length of one of their member, but i don't want to set it to 5000 characters either. If a guarantee can be made there (eg. "enum fields will never exceed 200 characters") that's awesome.

brandur-stripe commented 6 years ago

The main one that causes me trouble is enum fields. I don't want to set their max length to the maximum length of one of their member, but i don't want to set it to 5000 characters either. If a guarantee can be made there (eg. "enum fields will never exceed 200 characters") that's awesome.

Unfortunately I think most of these will probably show up as the default 5,000 right now. That's something we could fix at some point though.

jleclanche commented 6 years ago

Yep ok. Well, 5000 is better than nothing :) I will note that for the sake of the API contract, Stripe giving certain guarantees on field lengths would be good. I don't think this affects only dj-stripe, but really any library and code that stores outputs from the Stripe API.

In the end, these will be stored as varchar(5000) which, well, postgres doesn't care but it's not great.

brandur-stripe commented 6 years ago

Maximum lengths are now included for strings.

jleclanche commented 6 years ago

This is great, thank you! Hope to see them tightened up in the future, there sure are some weird ones in there :)

brandur-stripe commented 6 years ago

Agreed. I wouldn't hold my breath though :/ I wish we'd had tighter constraints in the beginning, but we didn't, and tightening them up now turns out to be quite difficult because there's a wide enough variety of usage that even changing maximum lengths will break integrations.

jleclanche commented 6 years ago

@brandur-stripe Understood. If I had to do it, I suspect I would make it a new API version, and require some migration when clicking "Upgrade API" if the account is under some limits.

I also suspect there's a lot of fields that could be migrated without issues, right? I mean, increasing max lengths could break integrations, but decreasing them wouldn't unless there's existing data in there.

For example, looking at address postcode, that's maxlength 5k; I doubt you have many of those :)

brandur-stripe commented 6 years ago

I also suspect there's a lot of fields that could be migrated without issues, right? I mean, increasing max lengths could break integrations, but decreasing them wouldn't unless there's existing data in there.

I think the opposite is true: increasing maximum length is perfectly fine because you're strictly relaxing a constraint. Decreasing it is often a problem though because we have users who might regularly pass in long strings in certain areas. I recently pushed through a project to decrease maximum string length from unlimited length for all strings in the API (gah, very unfortunate) to the current 5,000 characters in most places, and it turned out to be a lot more effort than I'd hoped. We had users sending in strings 10,000s of characters long in numerous places.

If I had to do it, I suspect I would make it a new API version, and require some migration when clicking "Upgrade API" if the account is under some limits.

For example, looking at address postcode, that's maxlength 5k; I doubt you have many of those :)

Yeah, there's definitely various techniques to make this possible and there will be at least a few places where fixing the strings would be relatively easy too. As this point it's more of a cost/benefit analysis problem — I'd like shorter strings, but in practice the long ones don't generally cause too much trouble, so the steady state of the present isn't too bad.

jleclanche commented 6 years ago

increasing maximum length is perfectly fine because you're strictly relaxing a constraint. Decreasing it is often a problem though because we have users who might regularly pass in long strings in certain areas.

Yeah, I understand what you mean. I'm thinking about it more in terms of a receiver than sender (receiving webhooks vs. sending api requests).

I also don't know that our string limits in dj-stripe, which are much lower than Stripe's in many places, have ever really been an issue. We also have ints in places where we should have bigints (like for example max redemptions on coupons). Nobody uses such large limits other than to battletest the Stripe api.

But if dj-stripe is ever unable to ingest data from a webhook, that's still a bug (and a serious one if it's in production rather than test mode). So it's a tough call.