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

Add RawJObject getter to StripeEntity #1970

Merged
merged 1 commit into from Mar 31, 2020
Merged

Add RawJObject getter to StripeEntity #1970

merged 1 commit into from Mar 31, 2020

Conversation

ob-stripe
Copy link
Contributor

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

Add RawJObject getter to StripeEntity. This method returns a JObject that can be used to access any properties contained in the JSON response, even if the library does not have a proper accessor for it.

This is similar to the escape hatch we have in stripe-java: stripe/stripe-java#799

@remi-stripe
Copy link
Contributor

@ob-stripe What's the difference with the approach we tried last year in #1707

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.

Approved but left some comments!

@ob-stripe
Copy link
Contributor Author

@ob-stripe What's the difference with the approach we tried last year in #1707

All fields are accessible, not just those that don't have normal accessors. So if someone uses RawJObject to access a field, and we later add a normal accessor for that field, their integration won't suddenly stop working.

@clement911
Copy link
Contributor

It's not new but one thing that's a bit strange is that IStripeEntity.StripeResponse is only populated on the top level entity returned by the API.
Same thing for this new RawJObject I guess.

I was thinking we could move them to a new IStripeRootEntity interface, only implemented by root objects.
However, for listable entities, they would be the root entity if Get is used but not if List is used (in that case the StripeList<T> would be the root entity). So it's probably not the best design.

Example showing the current issue:

new CustomerService().Get(id).RawJObject["SecretField"] => OK

new CustomerService().List().First().RawJObject["SecretField"] => NullReference exception

Ideally, there would be a way to populate the RawObject for ALL entities in the returned object graph. I wonder if the Newtonsoft can do that...

@clement911
Copy link
Contributor

Also not sure if you looked into System.Text.Json?

@clement911
Copy link
Contributor

I'm pretty sure a custom json deserializer would be able to set a JObject property on all entities.

@ob-stripe
Copy link
Contributor Author

Hey @clement911, thanks for all the feedback as usual :)

I think having the StripeResponse be available only on the top-level object returned by the API makes sense and is appropriate for all reasonable use cases I can think of, but I'm happy to hear about counter examples.

For RawJObject specifically, I do agree that it would be very convenient to have it available at every level (so you can use the "real" accessors up and keep type safety up until the point you can't), but I don't think we can accomplish that without a new custom deserializer as you mentioned, which I'd prefer to avoid. We have quite a few custom deserializers already and I'd rather not add any new ones if we can avoid it. Using RawJObject should be a "last resort" so hopefully this doesn't come up too often. That said, we can ship this as is and add support for RawJObject at every level as a future improvement.

System.Text.Json looks interesting, and I would definitely favor a standard library solution over an external dependency like Newtonsoft.Json, but it looks like it's only available as of .NET Core 3.0. Unless it's backported to .NET Standard 2.0, we will be unable to use it :(

Good point about the NRE, will add a check.

@ob-stripe ob-stripe merged commit d836d2b into master Mar 31, 2020
@ob-stripe ob-stripe deleted the ob-rawjobject branch March 31, 2020 01:53
@ob-stripe
Copy link
Contributor Author

Released as 35.11.0.

@sstalder
Copy link

sstalder commented Apr 3, 2020

This breaks my dotnet core 3 project not using Newtonsoft.Json as the serializer, and I would assume anyone using System.Text.Json.

This is due to the JsonIgnore attribute from Newtonsoft not being used by System.Text.Json as it has it's own attribute.

There is some more information under this section on the migration guide: https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to#conditionally-ignore-a-property

An unhandled exception has occurred while executing the request.
System.NotSupportedException: The collection type 'Newtonsoft.Json.Linq.JObject' on 'Stripe.Checkout.Session.RawJObject' is not supported.
at System.Text.Json.JsonPropertyInfoNotNullable`4.GetDictionaryKeyAndValueFromGenericDictionary(WriteStackFrame& writeStackFrame, String& key, Object& value)
at System.Text.Json.JsonPropertyInfo.GetDictionaryKeyAndValue(WriteStackFrame& writeStackFrame, String& key, Object& value)
at System.Text.Json.JsonSerializer.HandleDictionary(JsonClassInfo elementClassInfo, JsonSerializerOptions options, Utf8JsonWriter writer, WriteStack& state)
at System.Text.Json.JsonSerializer.Write(Utf8JsonWriter writer, Int32 originalWriterDepth, Int32 flushThreshold, JsonSerializerOptions options, WriteStack& state)
at System.Text.Json.JsonSerializer.WriteAsyncCore(Stream utf8Json, Object value, Type inputType, JsonSerializerOptions options, CancellationToken cancellationToken)
at Microsoft.AspNetCore.Mvc.Formatters.SystemTextJsonOutputFormatter.WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding)
at Microsoft.AspNetCore.Mvc.Formatters.SystemTextJsonOutputFormatter.WriteResponseBodyAsync(OutputFormatterWriteContext context, Encoding selectedEncoding)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|29_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()
--- End of stack trace from previous location where exception was thrown ---
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Logged|17_1(ResourceInvoker invoker)
at Microsoft.AspNetCore.Routing.EndpointMiddleware.g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
at IdentityServer4.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events)
at IdentityServer4.Hosting.MutualTlsTokenEndpointMiddleware.Invoke(HttpContext context, IAuthenticationSchemeProvider schemes)
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at IdentityServer4.Hosting.BaseUrlMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.MigrationsEndPointMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.Invoke(HttpContext httpContext)
at Microsoft.AspNetCore.Diagnostics.EntityFrameworkCore.DatabaseErrorPageMiddleware.Invoke(HttpContext httpContext)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

@ob-stripe
Copy link
Contributor Author

@sstalder Could you open a new issue about this?

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

5 participants