Skip to content

Feature/fluent mapping #771

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

Conversation

yaniv-yechezkel
Copy link

Added support for Fluent API for Resource Mapping

Including:
Resource Name
Attributes and Capabilities
Relationships
Eager Loads
Links

Extended EF Demo with Models and Resource Mappings

Fluent has Precedence when combined with Annotations
Demonstrate Fluent Resource Mapping
@wisepotato
Copy link
Contributor

what exactly is being added here? Why are there no tests?

@yaniv-yechezkel
Copy link
Author

I've talked with Bart about adding Fluent Mapping for Resources as an alternative for Annotations
Wanted your perspective regarding the approach taken so far...

@bkoelman
Copy link
Member

@yaniv-yechezkel Correct me if I'm wrong, but I understood that the goal here is to have no NuGet reference to JADNC from your Models project (onion architecture). Using fluent mappings would enable that.

My first thought is that I like the general approach you have taken. There's many little things to improve (including we need to have tests) which we can go over at a later time. For now, the AppDomain scanning looks expensive and I think it should accept a list of assemblies/types instead, coming from Startup.Configure method. Thanks for your efforts so far!

@maurei thoughts?

@yaniv-yechezkel
Copy link
Author

Thanks @bart-degreed.

Yes, The main goal it to keep the Domain clean.
So Fluent mapping can now replace all the Annotations, but I still have Identifiable, and it looks like it is coupled to Discovery, so it would be a challenge...

Providing a list of Assemblies would be much better, and Tests... of course!

Sure. It's a great project.

Enable Registration of specific Resource Mappings
@yaniv-yechezkel
Copy link
Author

yaniv-yechezkel commented May 25, 2020

@bart-degreed I've managed to handle Resource Mapping registration during Discovery.
So it only looks for types in the Assemblies from Startup, and there's also an option to register specific Mappings.
I had to play with Application Builder Setup to make things happen in the desired order.
Let me know what you think.

@bart-degreed
Copy link
Contributor

bart-degreed commented May 28, 2020

About IIdentifiable: see my comment at #730 (comment).

I've created an issue (#776) for the design this PR is supposed to implement. I'm curious about your opinion, so feel free to add comments/remarks/questions there.

@bart-degreed
Copy link
Contributor

Based on my proposal, I don't think we need to have assembly/type scanning at all. The building steps can simply be extracted into a separate class if the goal is to keep Startup.cs clean.

@yaniv-yechezkel
Copy link
Author

From my understating, there's already assembly/type scanning happening. This is the discovery.
I didn't add any new concept here, just relied on the current flow.
Startup remains the same. No changes at all.

@bart-degreed
Copy link
Contributor

You are correct that discovery also picks up resources, aside from services and definitions. This is all convention-based. I meant we should not add extra discovery steps (which you did at https://github.com/json-api-dotnet/JsonApiDotNetCore/pull/771/files#diff-fbd8f7ce570dc8180eb86d101f854b1dR249).

@yaniv-yechezkel
Copy link
Author

I totally agree with you there should not be an extra step.
Sorry, the link redirects me to JsonApiApplicationBuilder.cs, but I'm not sure as to which line are you referring...

@bart-degreed
Copy link
Contributor

I intended to reference this line in JsonApiApplicationBuilder:

_services.TryAddSingleton<IResourceMappingService>(sp => new ResourceMappingService(_services));

@yaniv-yechezkel
Copy link
Author

Thanks.

It is not a separate step.

It is used in Discovery for registering the Resource Mappings provided from Startup, and then for resolving Resource Mapping when building the Graph.

When Graph Builder is looking for defining Attributes, Relationships etc on a given Resource, it asks the Resource Service for available Mapping. At this stage Resource Mapping Service relies on them to already be registered on the Container...

@yaniv-yechezkel yaniv-yechezkel deleted the feature/fluent-mapping branch May 29, 2020 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants