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

Fix handling of null values with AnyOf #1661

Merged
merged 1 commit into from
Jun 12, 2019
Merged

Fix handling of null values with AnyOf #1661

merged 1 commit into from
Jun 12, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @remi-stripe @fred-stripe
cc @stripe/api-libraries

Fix handling of null values with AnyOf in the encoder. null values are now ignored, like they would be for a non-AnyOf property.

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this looks good to me. Is it worth adding a test for Source which is critical in this library so that we don't break it again? the created one is a bit of an edge-case and something we wouldn't care to remove the test for.

Also, is there a world in which you wanted to unset something and now it's broken (like how AdditionalOwners could be unset before)?

Sorry for the vague questions, I'm having trouble grasping whether it's possible for something else to be an edge-case.

@ob-stripe
Copy link
Contributor Author

Is it worth adding a test for Source which is critical in this library so that we don't break it again? the created one is a bit of an edge-case and something we wouldn't care to remove the test for.

Yep, good idea. I added a new ChargeCreateOptionsTest test class to test the various cases for ChargeCreateOptions.Source.

Also, is there a world in which you wanted to unset something and now it's broken (like how AdditionalOwners could be unset before)?

I think we're good:

  • list fields can be unset by setting the value to an empty list
  • metadata keys can be unset by setting the key to either null or an empty string (in this case, we do encode null as an empty string since we know for sure that the null value was explicitly inserted in the dictionary by the user)

Both cases are already covered in FormEncoderTest.

Can you think of any other cases for unsetting values?

@ob-stripe
Copy link
Contributor Author

ptal @remi-stripe

@remi-stripe
Copy link
Contributor

Can you think of any other cases for unsetting values?

There's this whole "to unset a hash, send an empty string" like to empty Charge.shipping you do shipping="". Does that still work? Or would it be introduced as a separate concept like we did the Empty in stripe-java?

@ob-stripe
Copy link
Contributor Author

There's this whole "to unset a hash, send an empty string" like to empty Charge.shipping you do shipping="". Does that still work? Or would it be introduced as a separate concept like we did the Empty in stripe-java?

AFAICT that was never supported in stripe-dotnet. It would be easy to implement something similar to stripe-java's EmptyParam.EMPTY with AnyOf though, but let's do that separately since it would be a new feature.

@ob-stripe ob-stripe merged commit a62936b into master Jun 12, 2019
@ob-stripe ob-stripe deleted the ob-fix-anyof-null branch June 12, 2019 23:39
@ob-stripe
Copy link
Contributor Author

Released as 27.1.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants