Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request: Include max length for string fields #26

Closed
jleclanche opened this issue Mar 18, 2018 · 14 comments
Closed

Request: Include max length for string fields #26

jleclanche opened this issue Mar 18, 2018 · 14 comments

Comments

@jleclanche
Copy link

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
Copy link
Author

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
Copy link
Contributor

brandur commented Mar 19, 2018

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
Copy link
Author

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
Copy link
Author

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

🤔

@brandur-stripe
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

Maximum lengths are now included for strings.

@jleclanche
Copy link
Author

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

@brandur-stripe
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

brandur-stripe commented May 31, 2018

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
Copy link
Author

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.

therefromhere added a commit to prismaticd/dj-stripe that referenced this issue Sep 10, 2019
Most stripe string fields should be 5000 chars wide (see stripe/openapi#26 )
Made Char/Text/Enumfields in new models non-null, since we force None to "" in StripeModel._stripe_object_to_record (so there should be no nulls on these fields even for people running vs master)
therefromhere added a commit to dj-stripe/dj-stripe that referenced this issue Sep 10, 2019
Most stripe string fields should be 5000 chars wide (see stripe/openapi#26 )
Made Char/Text/Enumfields in new models non-null, since we force None to "" in StripeModel._stripe_object_to_record (so there should be no nulls on these fields even for people running vs master)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants