-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
Extension for Gridify to be able to use it with Elasticsearch #126
Conversation
…functionality. Add tests for different types of data and operators with different combinations of complexity.
PR Summary
|
I will update the documentation in the |
Hi @ne4ta, |
Hi @alirezanet. Sure. Here you can see some examples:
It will return the collection of Users.
The query to Elasticsearch:
The query that will be sent to Elasticsearch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quickly checked your PR and overall it is awesome 🚀, but there are a few problems we need to fix before adding this.
- I noticed we have a lot of duplicate code which makes it really hard to maintain this later especially if users start using it, instead of coping the existing code would be better if you could make the internal APIs more flexible and reusable in a way to support your extensions.
- You didn't add any of these features to the
QueryBuilder
class which is the second way of creating queries in Gridify, - I saw in your example there is a manual paging logic, would be nice if you could add the
ApplyPagination
support to ElasticExtensions or evenGridify()
to do everything at once
.From((gq.Page - 1) * gq.PageSize)
.Size(gq.PageSize)
let me know what you think and even if you disagree with my comments, so we can talk about it openly. and again thank you for your contribution. 💐
public GridifyExtensionsTests() | ||
{ | ||
// Disable Elasticsearch naming policy and use property names as they are | ||
GridifyGlobalConfiguration.CustomElasticsearchNamingAction = p => p; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing decimal and double values is a Culture-specific operation, for example, on my machine, 18 tests failed because my decimal separator is a ,
, this is not a solution to fix this problem in Gridify, but for now add this to your tests to fix it in everyone's machines. (later we need to add CultureInfo support to gridify)
GridifyGlobalConfiguration.CustomElasticsearchNamingAction = p => p;
CultureInfo.CurrentCulture = CultureInfo.InvariantCulture; // <--
if (string.IsNullOrWhiteSpace(filter)) | ||
return new MatchAllQuery(); | ||
|
||
var syntaxTree = SyntaxTree.Parse(filter, GridifyGlobalConfiguration.CustomOperators.Operators); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some warnings that if you already have null checks like this example, it is easy to fix using a !
operator.
var syntaxTree = SyntaxTree.Parse(filter!, GridifyGlobalConfiguration.CustomOperators.Operators);
Github analyzer already marked these so I'll skip other ones.
/// If false CLR property EmailAddress will be inferred as "emailAddress" Elasticsearch document field name | ||
/// If true, the CLR property EmailAddress will be inferred as "EmailAddress" Elasticsearch document field name | ||
/// </example> | ||
public static Func<string, string>? CustomElasticsearchNamingAction { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Action is needed.
for now, it is okay to have this setting here but ideally would be better if we find another way to keep other extensions's configurations separated. (Just said this in case you could find a cleaner solution for this problem)
/// If false CLR property EmailAddress will be inferred as "emailAddress" Elasticsearch document field name | ||
/// If true, the CLR property EmailAddress will be inferred as "EmailAddress" Elasticsearch document field name | ||
/// </example> | ||
public Func<string, string>? CustomElasticsearchNamingAction { get; set; } = GridifyGlobalConfiguration.CustomElasticsearchNamingAction; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as global configuration
|
||
namespace Gridify.Elasticsearch; | ||
|
||
internal static class ToElasticsearchConverter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid duplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should note, I know how complicated this convertors are and also internal Gridify expression builder is not easy to read, so let me know if I can help on this or you had any questions. maybe this is a good reason also for me to clean up the internal APIs
Hi @alirezanet. Thank you for your review. I will try to improve the code as much as I can. |
…nd use them for LINQ and Elasticsearch query builders.
… the same API as original Gridify
Hi @alirezanet. I've updated the PR with all necessary changes. I also updated the documentation. Furthermore, I had to exclude tests from formatting in husky settings cause it broke my tests. Also, I wanted to ask you about versioning of Gridify.Elasticsearch. It uses Elastic.Clients.Elasticsearch 8.* and this is the lowest version of this lib. Previously, the NEST lib was the official Elasticsearch client for .NET. And the version of the official client corresponds to the version of Elasticsearch that is supported. Do you think it makes sense to start Gridify.Elasticsearch with 8th version as well? E.g. Gridify.Elasticsearch 8.2.11.1 where 8 is a version of Elasticsearch and 2.11.1 is a version of Gridify. Or maybe you have other thoughts about the versioning. |
Hi @ne4ta, Versioning:
Documentation:
Code:The code looks improved substantially, but I plan to review and test it shortly. I'll inform you if any changes are necessary. QuestionWould it be acceptable for me to commit changes to your branch if I come across any easily fixable issues during the review in your branch? Ultimately, I want to express my gratitude for your tremendous effort and innovative idea. I believe that this extension will become highly popular and beneficial for Gridify in the near future. |
Hey @alirezanet, Versioning:Yes, I agree with the same approach that is used in Gridify now. Let's have the same approach for all packages. Documentation:Sure, I will redo the doc. That can really be very confusing for the users. Question:Please feel free to do any changes in my PR if it does not require a discussion. And thanks for your kind words! I'm looking forward to the first release of the lib :) |
Hi @alirezanet. I've updated the documentation. I hope now it looks better. |
Hi @ne4ta, And about the extension methods, I think adding an extension method to a var query = "name = John".ToElasticsearchQuery<User>();
await client.SearchAsync<User>(s => s
.Index("users")
.Query(query)); Since this SDK doesn't support strings, why don't you directly add support for string queries? await client.SearchAsync<User>(s => s
.Index("users")
.Query("name = John")); This way, we're adding another extension overload to this method. public static SearchRequestDescriptor<T> Query<T>(this SearchRequestDescriptor<T> searchRequestDescriptor, string filter, IGridifyMapper<T>? mapper = null)
{
var query = ToElasticsearchQuery(filter, mapper); // we can make ToElasticsearchQuery internal or private
searchRequestDescriptor.Query(query);
return searchRequestDescriptor;
} EDIT: but on the other side this is the same as ApplyFiltering. 🥴so nvm. for now if you want to keep these two extensions, they should be added to the Let me know what you think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 💐🙌,
Everything looks good, I'll add some small changes to fix the Husky and formatter problems then I'll merge it 😎
Hi @ne4ta, FYI: Just to keep the versioning simple and avoid any confusion, I started with the same version as the Gridify base project. |
Hi @alirezanet, Great news! Thanks for your help! |
Hi @alirezanet. It looks like I didn't fully understand the concept of filtering using collections. I saw in the doc that you can use it like this
We definitely need to add the capability to search in such a way. I will implement this feature. I can't say how much time will it take, but I will do it when I have free time. |
Hi @ne4ta, let me know if I can help, also you can contact me on Discord if you have any questions. |
Description
Added an extension for original Gridify to work with Elasticsearch. The extension supports filter and sorting conversion to Elasticsearch DSL language queries.
Fixes # (issue)
Type of change
Checklist