Skip to content

Custom resource name is not being honoured /RFC #660

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

Closed
wisepotato opened this issue Dec 23, 2019 · 5 comments
Closed

Custom resource name is not being honoured /RFC #660

wisepotato opened this issue Dec 23, 2019 · 5 comments
Assignees

Comments

@wisepotato
Copy link
Contributor

wisepotato commented Dec 23, 2019

Issue

Currently when you add a Resource(..) attribute to a resource in the DbContext as such:

[Resource("weird-NaMiNg")]
public virtual DbSet<SuperNiceModel> SuperNiceModels { get; set; }

The name SuperNiceModels (the property name) is being used to create an URL and type identifier. The following identifier is used because of that: superNiceModels. What is the weird-NaMiNg identifier actually being used for?

Solutions

Two way to approach this:

First Version

If i add a Resource("cookiesMcCookieFace") To a mobel in my DbContext, I expect two things:

  • An overridden url /api/v1/cookiesMcCookieFace
  • An overriden type {'type' : 'cookiesMcCookieFace'}.

Second Version

If i add a Resource(url: 'cookiesMcCookieFace', type: 'cookie') To a mobel in my DbContext, I expect two things:

  • An overridden url /api/v1/cookiesMcCookieFace
  • An overriden type {'type' : 'cookie'}.

Thoughts?

@maurei
Copy link
Member

maurei commented Dec 23, 2019

A detailed description of the issue you're trying to resolve is missing here, please elaborate a bit on that. Currently other people will not be able to follow this RFC. [edit] thanks

Meanwhile I'm putting some thoughts in this.

@maurei
Copy link
Member

maurei commented Dec 23, 2019

So, for every resource, two things need to be calculated: (1) the type field value ( the {type: ...} bit ) and (2) the URL of the resource.

The name SuperNiceModels (the property name) is being used to create an URL and type identifier.

This is not true. Let's see how these are calculated right now. Enumeration is in order of priority.

Type field value for a resource:

  1. Using ResourceAttribute this can be customized, in which case the provided value is taken without further processing. (NB: can be applied in both the actual or associated DbSet property in DbContext).
  2. The default value is the name of the associated class, pluralized + case convention is applied (camelCase by default).

Resource URL for a resource

  1. In the controller associated to the class of the resource, usage of [DisableRoutingConvention] with [Route( ... )] will use that url without further processing
  2. The default value is the name of the class, pluralized + case convention is applied (camelCase by default). I.e.: by default the type value and url value for a resource match*
  3. in case that the URL that follows from the standard default value is already registered, it will form a url based on the name of the controller associated to that class as a fallback.

So, to expose fully expose the class FooBar as a resource named as fooBar, i.e. {type: barFoo} and url api/v3/barFoo, one would have to do the following two things

  1. use [Resource("barFoo")] on the model/dbContext
  2. use [Route("api/v3/barFoo")on the controller associated to theFooBar` class

This needs to be put in the migration guide upon v4 full release (and docs too in general). Also, this should be enough to solve your issues with with respect to how resources are exposed.

About your proposals regarding ResourceAttribute
I guess the question is if we want ResourceAttribute to control just the field type name or both the URL and field type name. Right now it is the former, and both your proposals suggest to change it to the latter.

First proposal: I think it is still good to have explicit control over the type field value without changing the URL with it. This proposal would eliminate that ability
Second proposal: This is basically the two actions ( (1) and (2) ) above brought to the Resource attribute, I don't think it adds a lot of added value as this the same effect can already be achieved as above. Also, when it comes to explicit URL definitions, I think it would be better to keep that at the controller level to stick with the .net core ideas of where to configure this.

But to elaborate on both proposals one and two: it would be good to be able to tell JADNC to not use the class name to calculate the default values of URL and type, but a different provided value instead. I agree that [Resource(..)] kinda suggests that this is what it does. Maybe we could introduce another attribute that specifically targets the type value like ResourceAttribute does now, and set the behaviour of ResourceAttribute as described now?

But given that it wouldn't introduce anything that can't be done right now, maybe not worth the hassle?

@wisepotato
Copy link
Contributor Author

After consultation with @maurei we came to the conclusion that we have multiple ways of doing this.

Solution 1

meta class buildder

meta.Define<DRCU>(entity => {
  entity.SetUrl("default-reports");
});

This is great long term, allowing us a central location to store configuration for the 'meta' part of the application.

Solution 2

Add the URL part to the ResourceGraphBuilder,

services.AddJsonApi(...,
  rg => {
    rg.AddResource<SuperNiceModel>(urlIdentifier: "weird-NaMiNg"); 
  });

we went with this ,as it is easy to implement, and is a great precursor for the eventual Meta class.

@maurei
Copy link
Member

maurei commented Dec 24, 2019

With respect to the urlIdentifier part of the approach, I've been giving it a second thought.
In JADNC v3.1 the url part was generated based on the controller:

// would expose the FooBar model as `{namespace}/foo-bar` 
public class FooBarController : JsonApiController<FooBar> { } 

// would expose the FooBar model as `{namespace}/custom-bar-foo` 
public class CustomBarFooController : JsonApiController<FooBar> { } 

Note that this is just a .NET Core off-the-shelve feature. The only thing that JADNC did was to apply a casing convention (.net cores default PascalCase to kebab-case). (Also note that ResourceAttribute never affected the url part in v3.1)

What happened in v4 is that the link between controller/url and associated link was registered internally so that we could populate the CurrentRequest service in the middleware rather than in the controller layer. I think an unwanted change there is that the calculation of URL changed: it moved from controller-based to model-based, i.e. CustomBarFooController now maps to /{namespace}/foo-bar. This is what brought about the unexpected routing changes in your projects. (And of course, also kebab-case was changed to camelCase).

I would be in favour of reverting this change and generate the URL based on the controller again to fix the issue your faced:

  • This way we would not introduce a breaking change and we would still mostly rely on off-the-shelve .net core routing instead
  • Still, we could still implement solution 2 as a feature to completely override the url identifier with a custom bit, which one could use if you wish to deviate from eg the casing convention.

@maurei
Copy link
Member

maurei commented Sep 17, 2020

Superseded by #786 and the associated PR. Decisions about this topic have been made there. More info about the current approach is described here

@maurei maurei closed this as completed Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants