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

Remove prefix validation from ids #530

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

mzeitlin11
Copy link
Collaborator

Sample implementation for #529. Fixes #529.

@arlyon
Copy link
Owner

arlyon commented Apr 8, 2024

Ideally, we would make this user-configurable. Can the parsing code check some global static that can be set? I am OK with making this opt-in, rather than out. The lack of errors is annoying but, for the core products, we are pretty safe. I would also like to instrument the code with tracing before 1.0 so a trace error on a failed ID would be handy.

EDIT: An example for when this might be useful is if people are using this library to store IDs in their database. Having type level validation is kinda handy. But aside from that TBH I don't really see many benefits so lets rip it out.

@mzeitlin11
Copy link
Collaborator Author

EDIT: An example for when this might be useful is if people are using this library to store IDs in their database. Having type level validation is kinda handy.

Yeah, that makes sense. We could also add another *Id constructor that explicitly validates the prefix is one of the expected ones if users want to opt-in to that additional safety. I just wanted to avoid the deserialization code relying on ids since it seems like a large compatibility hazard and any id coming from stripe should always be valid.

@mzeitlin11 mzeitlin11 merged commit 7686a0b into arlyon:next Apr 10, 2024
18 checks passed
@mzeitlin11 mzeitlin11 deleted the remove_id_prefixes branch April 10, 2024 03:33
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

Successfully merging this pull request may close these issues.

None yet

2 participants