Make SaveChanges call configurable

[crgolden]

Description

Right now, SaveChangesAsync is being called on every CreateAsync, UpdateAsync, and DeleteAsync call. As such, these methods actually do two different things:

1. Set the EntityState on the EntityEntry to Added, Modified, or Deleted.
2. Persist the new state to the store.

Proposal:

1. Create a public virtual async Task<int> SaveChangesAsync(TResource entity) method that actually does the saving.
2. Add an optional parameter to the CreateAsync, UpdateAsync, and DeleteAsync methods called bool saveChanges = true that could be set to false.

These changes would make the methods less expensive for bulk operations.

As an aside, I thought perhaps I would just override these methods and have them do everything except the saving. But they have become even more difficult to override in version 4. Suddenly, many of the protected methods they use have become private. It is somewhat confusing to allow the methods to be overriden by marking them virtual, but to then restrict the accessibility of all the helper methods they use to private.
...

Environment

• JsonApiDotNetCore Version: 4
• Other Relevant Package Versions: N/A

[maurei]

Thanks for opening up an issue and your feedback @crgolden

I have been considering to add such SaveChangesAsync to separate the two concerns you mentioned. However the one thing I am unsure about is the extent to which we would still be abstracting away the EF Core API with additional methods like these. An important purpose of the repository layer is abstracting away whatever ORM/data layer is used and I need to research if having SaveChangesAsync would still make sense when implementing a repository layer that relies on an alternative to EF Core. If that turns out not to be the case, adding SaveChangesAsync would effectively just tightly couple the repository API with the EF Core API which would defy its purpose

With respect to the inability to override virtual methods: it is still on the list to reconsider which helper methods should be exposed. Note that compared to v3 the amount of helper methods have significantly increased and this is due to the a lot of bug fixes and additions to comply with the json:api spec (eg central stuff like complete replace through patch wasn't supported: #530).

On one side I realise that exposing them will greatly contribute extensibility: I have noticed myself during beta-testing that porting my custom services was a bit tedious. On the other hand I am hesitant because these are really just internals of the framework that are more volatile, and exposing them would require us to ensure backward compatibility. Possibly we could clearly state that these helper methods are internals and no backward compatibility is guaranteed. I believe EF Core did that with some of their internals, however I'm not sure if they bumped their major version when changing such internals

Thoughts are welcome.

[crgolden]

Thanks for the response.

adding SaveChangesAsync would effectively just tightly couple the repository API with the EF Core API which would defy its purpose

I agree with you in that regard. As far as eager loading with Include - are there other ORMs that provide that? I have only noticed it with EntityFramework.

I think the best approach would be to add bulk operations. Using the EFCore.BulkExtensions or the MongoDB C# driver, for instance, this is possible.

I made a simple IDataService interface that very was easy to implement using both EF Core and MongoDbB. I also bundled SaveChangesAsync in the EF Core implementation because it didn't make sense to separate it out.

these are really just internals of the framework that are more volatile, and exposing them would require us to ensure backward compatibility

That is a thoughtful approach and I appreciate that. Ultimately, I think providing bulk operations would address all my concerns. I often am inserting hundreds of records at once and doing a save on each one is really a deal-breaker.

[maurei]

I agree with you in that regard. As far as eager loading with Include - are there other ORMs that provide that? I have only noticed it with EntityFramework.

I'm not sure if other ORMs provide clearly defined APIs to sideload data like EF Core does: probably not all of them. But even if they don't, one could still implement the Include API of the JADNC repository by eg (naively) loading the table/document of the related data and constructing the included data in-memory. I.e. semantically I feel like an Include can always be fulfilled, but I might be wrong

Ultimately, I think providing bulk operations would address all my concerns.

Did you use the json:api bulk extension in v3? It is removed right now but we are planning on adding it back ASAP because it indeed is really useful

I made a simple IDataService interface that very was easy to implement using both EF Core and MongoDbB.

I would love to learn more about how you're using mongodb with JADNC as it could provide a good starting point for a future JsonApiDotNetCore.MongoDb package

[crgolden]

Did you use the json:api bulk extension in v3?

As of now, I haven't been able to figure out how to keep the Authorization working. As you can imagine, that is a big problem in any production-level application... I did notice that this package bypasses it completely, and suggests using Resource Hooks instead. So I've been trying to use v4 for that feature.

But so far I haven't been able to get the Resource Hooks working properly, either. I made another issue about that: #615

Thanks!

[maurei]

I'm unsure how the json:api bulk extension is helpful to solving the authorization problem. Either way, I'll look into that issue right now