NotFoundResult throws NullReferenceException

[crgolden]

Description

When an id is not found on a GetAsync(id) request, the controller properly returns a NotFoundResult. However, this seems to break after that and what is actually returned to the client is a NullReferenceException.

Most concerning from a security standpoint is that the stack trace is returned.

The most common problem is when detailed internal error messages such as stack traces, database dumps, and error codes are displayed to the user (hacker). These messages reveal implementation details that should never be revealed.

Reference: Improper Error Handling

I updated my sample repo with a simple integration test (NotFound) that reproduces this behavior. Please advise if I am missing something.
...

Environment

• JsonApiDotNetCore Version: 4.0.0-alpha3
• Other Relevant Package Versions: not sure

[crgolden]

the stack trace is returned

I see where there is a method DisableDetailedErrorsIfProduction that is disabling these errors if the "ASPNETCORE_ENVIRONMENT" environment variable equals "Production", but not everyone's production environments set that variable to the same value.

The method is setting the DisableErrorStackTraces and DisableErrorSource properties on the JsonApiOptions class, but when I try to set the same properties on the JsonApiOptions class available in the AddJsonApi method, they aren't there. Could these options be added to setup for people whose production environments set the "ASPNETCORE_ENVIRONMENT" variable to something besides "Production" (like "prod", etc.)?

[maurei]

Hi @crgolden, thanks again for opening up an issue. Your feedback has been pretty helpful.

There's a few things going on here

Stacktraces in error results

It indeed seems that the settings for including stack traces in the error outputs is communicated throughout the framework using static fields on the singleton IJsonApiOptions service. I am not a great fan of that. As you're suggesting, I would prefer to have this configuration exposed as as a normal class members.

Unfortunately right now this static config is accessed in places where DI currently can't reach (extension methods of ModelState, non-DI-registered JsonApiException factories) and there is no easy quick fix for that. I'm going to tackle that later because we want to focus on the full release of v4 before reworking parts of the framework like these.

For now I've exposed the internally used .EnableDetailedErrors() which gives you more control over this option and which should support your usecase of 'prod'. See the startup in the example project

[maurei]

NullReferenceException

Thanks for pointing that out. I've identified the issue and working on a fix. Note that in your test you're expecting a payload, the json:api spec spec doesn't necessarily require the server to return a payload. In our current implementation we do however return an error object.

[maurei]

Alpha4 has been released and should be available in NuGet as a prerelease any minute

[crgolden]

@maurei Thanks so much for your explanations - they are very helpful.

I'll have to wait on trying out the new version. My workplace has not put .NET Core 3.0 into its internal CI pipelines yet. I realize that in order to use features like MVC, ASP.NET Core 3.0 is all-or-nothing now due to the Shared Framework and discontinuation of individual NuGet packages (like Mvc.Core). It may have been impractical for you guys to support ASP.NET Core 3.0 without targeting netcoreapp3.0.

[maurei]

@crgolden Indeed, it wasn't feasible to introduce such backward compatibility.

You can work around the null reference exception you encountered by relying on conventional routing, i.e. don't use [ApiController][Route(" ... ")][DisableRoutingConvention]. If you really need use that, you should be able to fix the null reference exception by overriding the methods of your controller that call NotFound() and replace it with NotFound(null) like I did here

[crgolden]

Ahh... ok. Thanks for the tip.

The reason I'm using the [DisableRoutingConvention] attribute is I'm relying on versioning via the URL path and my controller-level Route attributes with the "api/v{version:apiVersion}/[controller]" template seem to be ignored otherwise.

I have tried setting the namespace on the JsonApiOptions, but when I do that the versioning breaks. Maybe I'm missing something simple there.

Thanks again.

[maurei]

Interesting, I didn't know about the features addressed in that reference. This means you're currently using multiple versions for a particular route, right?

That currently won't work: the value of namespace in JsonApiOptions is added to the controller template "as is" and there is no interaction with ApiVersionAttribute. This is something we should look into, but won't be fixed for v3.1.

In your case I would recommend to stick to attribute-based routing and work around the NullReferenceException as explained in my previous post.

For better configurability of stacktraces in error responses, you can add an extension similar to this one which should be enough for your needs.

[crgolden]

@maurei Thanks for the response.

I did have a chance to try out the v4.0.0-alpha4 release. There were a surprising number of breaking changes. I have been able to work through most of them, but I would like to share a few observations:

1. Entity/Resource splits: I have read the numerous threads in your repo about why you removed this, but it was one of the better aspects of the library. As you know, I use versioning in my APIs. As it happens, the endpoints using this package are all under a separate version from the prior endpoints. Having DTOs that can easily be changed between endpoint versions is a huge benefit - otherwise any DB changes ripple out to all API versions. Being able to plug in AutoMapper in the Service layer was very beneficial to separating these concerns. And, looking at the code, it seemed fairly trivial to simply call a Map() function in that Service layer when appropriate. This level of maturity is something that has become expected with OData, for example (OData Model Configurations). I was very disappointed to see this removed.
2. The StringId property: Please make this virtual. In v3.1.0, I was able to implement IIdentifiable and just use the same implementation for StringId from Identifiable. The reason I did this was so I could add a JsonIgnore attribute - I would prefer the previous versions of the API were not forced to see an irrelevant StringId property in all responses. (If E/R splits were still available this would be a non-issue - see previous point). Now, with this version, the TypeHelper is yet another previously usable class that has inexplicably become internal. So I can't use the same implementation but just add the JsonIgnore attribute. If it was virtual, I could just override and call the base, but add the attribute. (I mean, I could just do exactly what TypeHelper is doing, but come on...)
3. Document.Data: For whatever reason, this property is no longer of type ResourceObject - it is now just an object. I'm sure there's some reason, but it means now when deserializing API responses I have to constantly cast it to ResourceObject to get access to the Attributes dictionary. Not a dealbreaker but puzzling.
4. Cors/Authentication/Authorization: Please add some optional parameters to the UseJsonApi extension like bool useCors = false, bool useAuthentication = false, bool useAuthorization = false - then call the corresponding extensions on IApplicationBuilder if they are set to true. According to the docs, "For most apps, calls to UseAuthentication, UseAuthorization, and UseCors must appear between the calls to UseRouting and UseEndpoints to be effective." I was able to keep them working by simply calling app.UseMiddleware<CurrentRequestMiddleware>() and app.ApplicationServices.CreateScope().ServiceProvider.GetService<IInverseRelationships>()?.Resolve() myself, but it would be nice to allow for these things to work with the extensions your provide (I couldn't call LogResourceGraphValidations because, of course, it's private).
5. Immutable properties: This one was painful. In v3.1.0, if a property used [Attr("", true)], it was just ignored when sent in a PATCH request. Now with v4.0.0-alpha4, if that property exists in a PATCH request, it causes a 400 Bad Request. That seems silly. So.. (1) I do a GET, (2) cast the Data property to a ResourceObject, (3) clear the Attributes dictionary (or manually remove each immutable property), (4) add the updated property, (5) reset the Data property on the Document to the dictionary I cast to earlier and (6) now it's ok to send it to a PATCH. Figuring out why I was suddenly getting 400s when all was good before definitely took some time.

The enhanced Resource Hooks and ASP.NET Core 3.0 support are nice, but using this new version comes at quite a price. Unfortunately, I wasn't able to find any "migration guides" to help overcome the numerous breaking changes. I have more observations but I'll end here as I'm interested to hear your thoughts.

[maurei]

Thanks for the in depth comment. I will reply to everything in detail, but some things I need to first consider carefully.

But starting with a few things for which the answer is more straightforward:

3. Document.Data

In v3.1 there existed two data types that represented a json:api response: : Documents for list responses (multiple List<ResourceObject>) and Document (singular) for a single ResourceObject response.

It wasn't necessary to have these two different classes for the same type of API response, and made it significantly hard to create (the start of) a json:api client. These classes have been merged into Document, which now contains public object Data which points to either (depending on the format of the response)

• Document.SingleData which is of type ResourceObject
• Document.ManyData which is of type List<ResourceObject>

If you know at precompile time response form (single or many) your API, you can just use SingleData or ManyData (see example. If the usage is dynamic you can use Data and use is-patterns.

.... now when deserializing API responses ...

Note that v4.0 introduces easier (de)serialization: for client-side (de)serialization (eg in functional tests) you shouldn't have to touch the Document class at all. See this e2e test as an example for how to use client deserialization

information guide

This is still work in progress: before releasing the full version there will be an extensive document

Feel free to share those other thoughts you have. I will reply to everything eventually

[crgolden]

@maurei Thanks so much for your response. I want to start off by saying I think the library is great, and I appreciate all the work you guys are doing. If I didn't feel that way, I wouldn't take the time to share these thoughts.

I appreciate the clarification on the difference between Document.SingleData and Document.ManyData. That explains it well for me.

As for the client serialization - thanks for pointing out the example. In that case, I see you are doing some work in the protected GetDeserializer method that includes a combination of getting services from DI and also instantiating new instances there. If I may, I'll continue my observations from above with a suggestion here:

6. Document class: I think it would be a great addition to add two methods to this class:

• Document FromIdentifiable<T>(T identifiable) where T : IIdentifiable
• IIdentifiable ToIdentifiable()
  I think the majority of the work has already been done in your abstract BaseDocumentParser class - and it would not be difficult to move it to the Document class itself. This would be a great example of DDD (how a Document is parsed is in the domain of the Document itself).

7. Identifiable class: Is there any reason why this class is not abstract? Should it ever be instantiated on its own? And, following from the above suggestion, it would be a great addition to add two methods to this class:

• T FromDocument<T>(Document document) where T : IIdentifiable
• Document ToDocument()
  It would be a great help in deserialization and formatting to have these methods available for any class that inherits Identifiable. And, with C# 8, you can also add them to the IIdentifiable interface with default implementations.

8. Abstractions: I am a big fan of the abstractions pattern. I would really like to see the abstractions in this package published as a separate NuGet package. This would go a long way to supporting the clean architecture pattern. That pattern states that the entities should not be dependent on any implementations. I've alluded to this before when mentioning the difficulty I faced using entities that were decoupled into a separate assembly. As it is now, I have to add this entire library to the entities project just to reference the IIdentifiable abstraction. This means my entities have to depend on the full ASP.NET Core framework! Pulling the abstractions into a separate NuGet package is a great practice that Microsoft exemplifies well. By only depending on the abstractions package, my entities can remain ignorant of implementation details and only exist to model the domain.

9. Internal confusion: I've mentioned before that it's confusing to see previously available methods and classes being suddenly made inaccessible. While I do understand the need to decrease the surface area of the API to reduce breaking changes, I am somewhat confused by what's internal and what's not. For instance:

• to inherit from ResourceDefinition, I have to reference Internal.Contracts
• to inherit from DefaultResourceRepository, I have to reference Internal.Contracts and Internal.Generics
• to inherit from DefaultResourceService, I have to reference Internal.Contracts
• to add a controller attribute like [ProducesResponseType(Status400BadRequest, Type = typeof(Error))], I have to reference Internal
  And on and on. I'm not opposed to having things be internal when necessary (Entity Framework is a prime example), but it's confusing to have methods/classes arbitrarily being made internal while also having to use "Internal" namespaces all over the place. (By the way, is there a reason why base classes like ResourceDefinition, DefaultResourceRepository, DefaultResourceService, BaseJsonApiController, etc. are not abstract? Should they ever be instantiated directly?)

10. Produces/Consumes: Speaking of controller attributes, I've noticed they're not all respected. As previously mentioned, I have multiple versions of the same controllers. While one version may have an action decorated with [Produces("application/json"), Consumes("application/json")], another version may have the same action decorated with [Produces("application/vnd.api+json"), Consumes("application/vnd.api+json")]. But I find that for the first version, unless I explicitly specify "application/json" in my HttpContent objects, I get 415 (Unsupported Media Type) errors. This shouldn't happen because I've explicitly said that action on that version of the endpoint consumes "application/json".

11. JsonSerializerSettings: Another thing I'm having difficulty with are global JsonSerializerSettings. Because of how Entity Framework loads related data, I need to ignore cycles when serializing. So, in addition to adding SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore to the IServiceCollection AddJsonOptions method, I also add it to the IServiceCollection AddJsonApi method (options.SerializerSettings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore). But I find this is not respected at runtime and I still get a JsonSerializationException. The only way that works is to add JsonConvert.DefaultSettings = () => new JsonSerializerSettings { ReferenceLoopHandling = ReferenceLoopHandling.Ignore } in the constructor of every single controller. It's painful.

12. I know I mentioned at the beginning of this post how concerned I was that stack traces were being exposed by default. I would strongly suggest switching this to be an opt-in behavior. In other words, stack traces should never be exposed unless explicitly configured that way. This would be a great example of Secure by default. You can see this practice at work in ASP.NET Core itself - in the example application we always see if (env.IsDevelopment()) { app.UseDeveloperExceptionPage(); }. The docs call this out: "Enable the Developer Exception Page only when the app is running in the Development environment. You don't want to share detailed exception information publicly when the app runs in production."

13. Error objects: It would be great to have the ability to expose an id property on the Error class. I know the spec allows for this, and in my case my employer requires this. I don't know how you would implement this, but just adding properties to the Error class that mirror what the spec allows for would be a great start.

Thanks again for all your hard work. I look forward to hearing your thoughts.

[maurei]

Hi @crgolden, thanks again for the extensive feedback. Generally speaking: all of your feedback (of both your earlier and last post) is valid. Some points require further investigation, other elements could be incorporated straight away.

Before addressing each issue individually there is a comment to be made related to most of your points all together. Commenting on why many of these issues haven't been tackles yet: it pretty much boils down to (having limited) time. I've been focussing on issues I encountered myself while trying to use the library for several projects, and these have taken me several months to work on. As of the current pre-release of v4 I've tackled most (not all) of those issues, freeing up time for other improvements. Your feedback is extremely helpful in scoping out what to focus on beyond the current prerelease because your usage is different from mine: you have been probing parts of the framework that I haven't had the opportunity to thoroughly probe yet.

Still, the list of feedback is growing faster than I seem to be able to currently process, so I think it would prove helpful to split up this thread into separate and more actionable issues and move on from there. Also, with this I hope to speed up the overall development by promoting other developers contributions apart from my own.

I will reply on all of your unaddressed points with a brief comment indicating why I think if it

• ✅ can be directly incorporated
• ❓ needs further investigation.
• ❌ shouldn't be addressed for now

From there on we should continue the issues in a separate thread. Eventually I will do that gradually when time frees up, but you can do the same while weighing in the relative priority you attach to each issue.

[maurei]

1. Entity/Resource splits (❓)

Having ER splits as a use-case for versioned APIs is an interesting one and I would like to further investigate if ER splits is the only / the best way to go about this when using JANDC. What we need to investigate primarily is the complexity it will (re-)introduce in the codebase.

Note that in principle I am a strong proponent of being able to separate concerns through E-R. But
note that supporting this is significantly more complex than just calling a MapIn( .. ) and MapOut( .. ) at the beginning/end of each service pipeline. Using ER split will cause off-the-shelve features to rapidly break down if not carefully considered. For example:

• supporting ?filter[x]=y query parameters with ER split is not straight forward: if the resource and entity models are significantly different, then having an AutoMapper does not trivially translate a filter expression tree for resource into one for a expression tree for a (strongly deviating) entity. I feel having proper generic support for this is very difficult and will introduce a lot of assumptions/conventions, and if not carefully implemented it will strongly couple the service and repository layer. (It seems that in the v3.1 codebase ER split was even completely ignored in the GetAsync() pipeline (no MapIn and MapOut called here?), demonstrating how E-R split was never fully supported in the first place )
• More generally speaking: I think we can pretty safely assume that (even in v3.1) a large number of use-cases of JADNC will not work out of the box with ER splits. To get an idea of the size of that number: any feature that is tested in JsonApiExampleProject but not in the EntityResourceSeparationProject might not work.

2. The StringId property: Please make this virtual (✅)

I don't see any problems with having it virtual.

3. Already addressed (❌)

4. Cors/Authentication/Authorization (✅)

In general we should allow for full configurability of .net core 3 as much as possible. For standard usage it is nice to have a UseJsonApi() that takes care of everything, but we should allow for full customizability at all times.

5 Immutable properties: (✅)

• this change of behaviour is breaking and should be documented in the migration guide. I'm sorry for the rabbit hole you had to dive into to work out the details of this.
• I think being explicit and throwing an error when a client violates the writability of an attribute is preferable over silently ignoring such operation.
• Nevertheless, I also believe configurability is key, and we could allow for the developer to set this behaviour both globally (eg in serializer behaviour) as well as locally (eg [Attr( .. , canWrite: false, errorWhenWrite: true)]. I would propose this to be the way forward

6 & 7: (De)serialization + domain driven design (❓)

I like the idea of domain driven design, but I see a potential problem with extensibility. Eg being able to call Document ToDocument() on your model instance, the instance will need to access the serialization services which will require your model to be registered with the DI container. This would turn your model into an injectable service, and I doubt that is a good thing. We could avoid this by manually instantiating the (de)serializers in the models instead of using dependency injection, but then if one were to register a custom implementation of a (de)serializer this would not be picked up, i.e. there would be little extensibility. We need to look into to what extent this would pose a problem by investigating the use-cases for such methods. Maybe for client (de)serialization such addition would be still be helpful

8. Abstractions pattern ✅

I am a strong proponent of splitting up JADNC into several smaller NuGet packages. This would also allow for the following

• use a different serialization convention with the current service/repository structure of JADNC, see (RFC: Separate Concerns of JsonApiContext #253 (comment) and RFC: Separate Concerns of JsonApiContext #253 (comment))
• use eg MongoDB instead of EF core (see Separate EntityFramework APIs into separate package #292).
• use the core of JADNC as a client instead of a server.

Obviously this is a very big thing to tackle

9. Internal confusion ❓

The namespaces right now are still a mess and I completely understand your confusion. This needs tidying up. The same goes for which properties and methods are pubic/private/protected: all of this isn't properly thought through yet. We need to investigate what is the best way to organise all of this while keeping good extensibility. A way forward could be:

• anything that extensible in non-internal namespaces must be guaranteed to be backward compatible
• anything in internal is still exposed for extensibility but is not guaranteed to be backward compatible
  This way we could eg expose TypeHelper in an internal namespace to make live easier for developers, but at own risk and therefore without being bound by backward compatibility.

10. Produces/Consumes ❓

We should try to respect as much as possible the toolkit that is provided by .NET Core 3. We must be explicit about which features of .NET Core 3 are not supported: eg the middleware currently strictly requires EndPointRouting, and I think it is OK to enforce a design consideration like this (although we need to clearly state a motivation for it).

With respect to these attributes you mention, we have to investigate whether they are compatible with JADNC internal design and if we want to support them (hence the ❓), but I expect that the answer will be a ✅.

11. JsonSerializerSettings ✅

Nothing to add. Would like to understand better where exactly this error is triggered, but I expect this issue is actionable straight away.

12. Stacktraces ❌

If I'm not mistaken this has been solved already in the current pre-release with the usage of app.EnableDetailedErrors(). The default value for hiding stacktraces and error sources have been changed. Let me know if this does not behave as expected

13. Error objects ✅

No objections. Right now the class that constructs error objects is static and not registered with the DI container. To give more control / allow for more extensibility over how the error object is constructed we should register a error factory service with the DI container.

Thanks again for all your feedback. I look forward to hearing your thoughts

[crgolden]

@maurei Thanks for the terrific responses. I really appreciate you taking the time to address each issue I raised. And I agree that it would be a good idea to separate the split the thread. As far as I'm concerned, this particular issue (the null reference bug) has been addressed, so I will re-close it.

I would like to make one more general observation, however. I have seen multiple posts expressing a desire to increase the adoption rate of this library. I agree that would be beneficial - indeed, a concern expressed at my workplace for even using it has been related to the low adoption rate. And additionally, I know may of my observations have been around the issue of versioning. I believe these two are related.

This library potentially reshapes the entire public facing contract for each resource it's implemented on, often in a breaking way. As few of us are fortunate enough to live in a greenfield world, this often warrants a new major version of the API. So I think making versioning a first class concern would aid in increasing the adoption rate - especially among more mature organizations.

That being said, I was able to address a few of my concerns by adding this to my Configure method:

app.UseWhen(
    context => context.GetRequestedApiVersion().MajorVersion == {jadnc_version},
    builder => builder.UseJsonApi(false));

As OpenAPI is becoming more mature and adopted in the industry, I think addressing some API explorer concerns would also be beneficial in driving the adoption rate. For instance, as it is now I have to create a mix of SchemaFilter, DocumentFilter, and OperationFilter classes to properly show examples of the JSON API Document objects being returned when the JSON API version of the Swagger page is viewed.

To address this (and I believe make the library much more usable), I would propose this architectural change: to make all the controller actions receive Document objects and return Document objects in the IActionResult. The controllers could still be generic on T and TId (to know how to handle the Document objects), but this would be a more honest endpoint configuration.

I haven't dug into the code too much, but I'm guessing this would remove some complexity in the middleware. It would also make API explorers like Swagger work out-of-the-box with no filter madness.

Thanks again and have a great holiday.

[maurei]

I know many of my observations have been around the issue of versioning. I believe these two are related.

I think that is a legitimate observation and would be willing to prioritise development resources to this cause. At the same time at my workspace the demand for such feature is limited and for that reason it would beneficial for the framework if people were to be involved that have an in-depth overview of what precisely is required from JADNC to properly support versioning of APIs.

That being said, elaborating on the following

indeed, a concern expressed at my workplace for even using it has been related to the low adoption rate

The up-side of the low adoption rate is being able to easily influence the direction of the framework. I'm guessing my hint will be clear by now, having you guys contribute to this development would be more than welcome 👍🚀

I would propose this architectural change:

This is an interesting approach and definitely something we could investigate. First question that comes up in my mind is the following. The Document class is very specific to the json:api format, and if we make that dependency explicit in the controller layer, I'm not sure how we could use the core of JADNC with a different serialization format (eg XML) without having to switch to an entirely different controller layer (whereas it is really just the (de)serialization approach of the payload that needs to be switched).

[wisepotato]

@crgolden Chris, thanks for your effort to indicate problems with JADNC, a lot of points are valid.