Skip to content
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

Implement QueryProvider for converting from IQueryable to Json string #20 #23

Merged
merged 10 commits into from
Jun 1, 2023
Merged

Implement QueryProvider for converting from IQueryable to Json string #20 #23

merged 10 commits into from
Jun 1, 2023

Conversation

leo-schick
Copy link
Contributor

@leo-schick leo-schick commented May 20, 2023

Closes #20

I decided to do a redesign of the original version. The reason for this was that I found the implementation at some parts not so flexible / at some parts even dangerous to use.

Here a list of changes I did against the version from the Core API:

High level changes

  • the API client should be provided against the interface IApiClient which implements synchronous and asynchronous commands. The API requests are should implement the IApiRequest interface.
  • The IQueryable<T> class can be retrieved via the extension method GetAsQueryable (I am not 100% happy with this name)
  • when performing a query, the data is now requested on demand and not upfront
  • the default page size is set to 100
  • the limitation of max. 5 nestings has been removed (I did not see the point in this, but we could add it again when needed)
  • the expression parsing works now inside the ExpressionVisitor class capacity, allowing more flexibility in queries (=> client side executioning of not implemented Linq functions is possible). We could disable this or make it optional if wanted.
  • the support of method call ToString insinde an expression has been removed. (I could not find a meaningful example but when there is, I am happy to add it back again.)
  • a testing suite with mockup API client is available for testing the query/expression parser. Multiple tests have been implemented.
  • async. support added (⚠️ not tested, unit tests missing)

Low-level changes

  • the ApiQueryBuilder class has been copied and slightly modified
  • a new class ApiPagedEnumerable is now performing the actual API requests holding the ApiQueryBuilder instance
  • the ApiQueryProvider class is now only required for creating the ApiPagedEnumerable instance and parsing the expression tree.
  • the ApiQueryRewriter has bee implemented into ApiQueryProvider and is dropped
  • the provider behind IQueryabe<T> only supports enumerable requests. Single row calls are not supported anymore. (I am not sure if they where used in the Core API)

Implementation matrix

When it is not implemented, the default behavior applies --> meaning: the query is performed on client side, possibly running heavy API loads against the API.

IQueryable function Implemented Unit tests exist
Aggregate No No
All No No
Any No, could be implemented as a single record request No
Append No No
Average ✔️ No
Cast No No
Chunk No No
Concat No No
Contains No No
Count ✔️ No
DefaultIfEmpty No No
Distinct No No
DistinctBy No No
ElementAt No, easily possible No
ElementAtOrDefault No, easily possible No
Except No No
ExceptBy No No
First No, easily possible No
FirstOrDefault No, easily possible No
GroupBy No No
GroupJoin No No
Intersect No No
IntersectBy No No
Join No No
Last No, easily possible No
LastOrDefault No, easily possible No
LongCount ✔️ No
Max ✔️ No
MaxBy No No
Min ✔️ No
MinBy No No
OfType No No
Order No No
OrderBy ✔️ ✔️
OrderByDescending ✔️ ✔️
Prepend No No
Reverse No, easily possible No
Select ✔️ ✔️
SelectMany No No
SequenceEqual No No
SequenceEqual No No
Single No, easily possible No
SingleOrDefault No, easily possible No
Skip ✔️ No
SkipLast No No
SkipWhile No No
Sum ✔️ No
Take ✔️ No
TakeLast No No
TakeWhile No No
ThenBy ✔️ ✔️
ThenByDescending ✔️ ✔️
Union No No
UnionBy No No
Where ✔️ ✔️
Zip No No

@leo-schick
Copy link
Contributor Author

leo-schick commented May 20, 2023

@piotrczyz I is now ready for review and I think it is time to test it against an real api endpoint, not just the mockup API.

@u12206050
Copy link
Member

Nice work,
The 5 nesting limitation is to not overwhelm the server with a complex query, although I do think 5 might be to small for some so something like 6-8 could be a good max. I wouldn't recommend allowing more than that 8

In regards to the toString function, you say you removed it within the expression, so just wonder how you can debug or log the expressions nicely without it?

@leo-schick
Copy link
Contributor Author

I did not know that ToString was used for debugging - but when this is the case, I would rather suggest others ways:

  • when you want to know more about the expression tree, use DebugView. See also Debugging expression trees in Visual Studio
  • In case you want to see the final Expression Tree, I could add a function which allows publicly calling ApiQueryProvider.Visit (from ExpressionVisitor) and then you can take a look at DebugView. That would then show the expression which runs on client side without seeing the API calls and the content of ApiQueryBuilder which holds the parameters for the API calls e.g. the jsonFilter string.
  • When it comes to debugging the API calls, I would say we implement a global parameter and when set, the API calls are written to the Debug console. Similar to what you can do with EF Core, see here. This would then be implemented into ApiPagedEnumerable which performs the actual API calls.

@u12206050 would that be fine with you?

Copy link
Member

@piotrczyz piotrczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to discuss the implementation as I found it more complex than the solution created in Core API. Interesting to hear from you which part of Core API was not flexible enough.

tests/RuleFilterParser.Tests/Seeds/Persons.cs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
/// <param name="path"></param>
/// <typeparam name="T"></typeparam>
/// <returns></returns>
public static IQueryable<T> GetAsQueryable<T>(this IApiClient apiClient, string path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example in Core.Api suits better as it creates a client as a transient scope.

            services.AddHttpClient();
            services.AddTransient<ICoreApiClient>(x =>
            {
                var clientFactory = x.GetRequiredService<IHttpClientFactory>();
                return new CoreApiClient(options, clientFactory);
            });
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Persons );
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Countries );
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Relations );
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Affiliations );
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Consents );
            services.AddTransient(x => x.GetRequiredService<ICoreApiClient>().Orgs );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I see we need something like DbSet<T> for the API...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check out my new version of the sample ApiClientMockup class.

I renamed now the extension method from GetAsQueryable to GetQueryable to have the same naming as the Core API. But the generic type parameter and the path parameter are still required.

I am not anymore sure if we should keep the extension method. It would be possible that the ApiClient holds a instance of ApiQueryProvider as well. The static typed properties could instanciate a ApiQueryable instance on calling. I did not spend so much time on how the queryable logic should be integrated with the API client... maybe someone has better ideas

tests/RuleFilterParser.Tests/LinqTests.cs Outdated Show resolved Hide resolved
src/RuleFilterParser/Api/IResult.cs Outdated Show resolved Hide resolved
src/RuleFilterParser/Api/ApiRequest.cs Outdated Show resolved Hide resolved
src/RuleFilterParser/Api/ApiRequest.cs Outdated Show resolved Hide resolved
@leo-schick
Copy link
Contributor Author

It would be nice to discuss the implementation as I found it more complex than the solution created in Core API. Interesting to hear from you which part of Core API was not flexible enough.

Yes, we can arrange a meeting for that. Lets talk on discord about it.

Just some lines to explain myself here: The comment not flexible enough was mostly meant how the linq/expression parser was implemented. For example, combining API data and non-API-data like this

IQueryable<T> persons = api.Persons;
Dictionary<string, bool> euCountries = new Dictionary<string, bool>
{
  {"NO", false}.
  {"PL", true},
  {"DE", true}
};

// sorry for running simplified linq here ...
var query = from p in persons
  join euCountry in euCountries on euCountry.Key equals p.Country
  where euCountry.Value == true
  select p;

would never be possible. Now it should be possible (even tho I did not implement it yet). This is made possible because we visit the expression tree and replaces the parts in place which the API can handle. The other parts just remain in the expression tree and are client evaluated. (This might end up in may API calls ... but that is something we need to bring up somewhere else I think. I think w should not limit the developer functions but rather the number of allowed API calls when it gets too heavy).

It had some other dangerous stuff as well, e.g. when you run

IQueryable<T> persons = api.Persons;

var firstPerson = api.Persons.FirstOrDefault();

the Core API logic would just download all the persons (27k?) and then return a single entity. Now this is processed by the ApiPagedEnumerable class which would invoke just a single page. (It can be even improved by just calling a single page with limit 1, therefore I mentioned that FirstOrDefault is easy to implement, see the implementation matrix.)

Let us discuss it further in a meeting :-)

README.md Show resolved Hide resolved
@piotrczyz piotrczyz merged commit 08488af into bcc-code:main Jun 1, 2023
1 check failed
@leo-schick leo-schick deleted the query-provider branch June 23, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement QueryProvider for converting from IQueryable to Json string
4 participants