Optimize Queries

[btecu]

There are a few improvements that can be done to the auto generated endpoints:

• GET endpoints could have .AsNoTracking() added to the query
• DELETE endpoints could do .Attach() -> .Remove() instead of .Get() -> .Remove() to avoid making two calls
• UPDATE could possibly be improved similarly to DELETE

[jaredcnance]

Very good points!

#221 was opened to address your first point and was implemented in #225. However, I closed the PR because I didn't have cycles at that time to complete it. From the PR:

This is on hold until I can evaluate the scope of the breaking changes. Not only are there API implications but also impacts on possible extensibility scenarios. By setting the query as NoTracking, acceptance tests using the same context may be impacted.
Need to put together documentation on these breaking changes and how users should handle acceptance testing that uses the same DbContext instance.

Update
I think the best way to move forward is to allow a global options flag to be set in Startup that will enable AsNoTracking. It will need to default to false to avoid breaking changes and will allow tests to enable tracking for all queries.
There may also be impact to #238

In retrospect, based on experience with other projects (including DotnetTestAbstractions), I think tests should not share the same DbContext (in most cases -- but there is definitely a notable performance benefit) and should instead share the underlying connection and transaction. So, it would be less of a concern. However, it should still be configurable. We would release it disabled at first and then enabled by default in a later major version.

[btecu]

How about having GetRawAsync(bool isReadOnly) and be called from the other endpoints?

Somewhat related, better naming could be nicer. Looking at Get() and GetAsync() is not obvious the difference. Both are async but the first returns a collection while the second one single record.

[jaredcnance]

I think your reference to GetRawAsync is actually the intent of Get.

Get is not async. It doesn’t actually run a query since it returns IQueryable. It serves as the foundation for (most) other requests.

[btecu]

Right, it could be called GetRaw(bool isReadOnly).

[jaredcnance]

I'm open to discussion on the name change. For me, "GetRaw" doesn't actually clear the situation up any and begs additional questions. Perhaps just GetQueryable?

The rollout might look like:

// 1st release
[Obsolete("Use GetRaw instead", error: false)]
IQueryable<TEntity> Get() => GetRaw(isReadOnly: false);
IQueryable<TEntity> GetRaw(bool isReadOnly = true);

// 2nd (major) release
[Obsolete("Use GetRaw instead", error: true)]
IQueryable<TEntity> Get()  => GetRaw(isReadOnly: false);
IQueryable<TEntity> GetRaw(bool isReadOnly = true);

// 3rd (major) release
// gets removed in a major release so that someone jumping from 2.x -> 3.1
// will still get an informative error message
// jumping from 2.x -> 4.x will just break...
IQueryable<TEntity> GetRaw(bool isReadOnly = true);

The only thing here is that it makes global configuration more difficult which is what is needed for some testing scenarios (e.g. in TestStartup, you would set UseAsNoTracking = false if you're sharing the DbContext with your test).

Another option that would address this is:

IQueryable<TEntity> GetRaw(bool? isReadOnly = null);

In the null or unspecified case, the global default would be used. This supports global and per-resource configuration.

[btecu]

I like GetQueryable.

Since version 3 is beta, does it have to go through all those releases?

[jaredcnance]

I think it should. Because if we skip straight to the breaking change, then developers will upgrade to 3.0.0 with no prior warning and may have to change a significant amount of their code base (albeit find and replace should be pretty straightforward). I prefer to deprecate first when possible. Step 1 and 2 can probably be combined though. And step 3 should actually keep the Obsolete attribute and simply set error: true

[wisepotato]

@maurei status on this?

[bart-degreed]

Closing this, because:

• AsNoTracking is tracked at Use AsNoTracking on Readonly queries #221
• Delete-without-fetch is implemented by Composable filters and deeply nested queries #792
• Update-without-fetch makes it impossible to track side effects, which is required by the json:api spec.