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

Don't create AnyOf instances with null values #1702

Merged
merged 1 commit into from
Jul 10, 2019
Merged

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Jul 9, 2019

r? @brandur-stripe
cc @stripe/api-libraries

Modifies the implicit operators used to create AnyOf<> instances from value types to return null instead of creating a new AnyOf instance that contains null when the value is itself null,

Previous behavior:

AnyOf<int, string> anyOf = null;
anyOf = (string)null;
Assert.NotNull(anyOf);
// anyOf is now a (non-null) AnyOf<int, string> instance with `Value` set to `null`

New behavior:

AnyOf<int, string> anyOf = null;
anyOf = (string)null;
Assert.Null(anyOf);
// anyOf is still null

I've also changed ToString(), as previously it would crash if Value was null.

@@ -103,14 +103,14 @@ public override Type Type
/// </summary>
/// <param name="value">The value to convert.</param>
/// <returns>An <see cref="AnyOf{T1, T2}"/> object that holds the value.</returns>
public static implicit operator AnyOf<T1, T2>(T1 value) => new AnyOf<T1, T2>(value);
public static implicit operator AnyOf<T1, T2>(T1 value) => value == null ? null : new AnyOf<T1, T2>(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, because this is a generic class, I don't think it's possible to write a helper method to do this in a single place.

Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

OOC, was there some particular impetus for this, or did you just notice recently and decide to try to make it more correct?

Regardless, seems like a good improvement — LGTM.

The PR description examples were super helpful BTW — thanks!

@ob-stripe
Copy link
Contributor Author

OOC, was there some particular impetus for this, or did you just notice recently and decide to try to make it more correct?

A user ran into this when writing tests for their Stripe integration.

The previous behavior was not strictly a bug since the library would omit the parameter in the request in both cases (whether the AnyOf property is null or is non-null but contains a null value), but they were surprised that they were getting back a non-null value after explicitly assigning a null value.

@ob-stripe ob-stripe merged commit fe0dd67 into master Jul 10, 2019
@ob-stripe ob-stripe deleted the ob-anyof-null branch July 10, 2019 00:36
@ob-stripe
Copy link
Contributor Author

Released as 27.9.1.

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

3 participants