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

Fix GetTokenFilterDescriptor() in ElasticIndexManager #16974

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Nov 8, 2024

No description provided.

@hishamco hishamco requested a review from Skrypt November 8, 2024 13:18
@MikeAlhayek
Copy link
Member

@hishamco in PR's like this, it would be very helpful to see what issue is being fixed (link it to an issue).

From the look of it using property.PropertyType is better than using string. maybe PropertyType is an integer or any other type. Why string is better that the actual type?

@MikeAlhayek MikeAlhayek reopened this Nov 11, 2024
@MikeAlhayek
Copy link
Member

sorry closed accidentally.

@sebastienros
Copy link
Member

@hishamco Any more info?

@hishamco
Copy link
Member Author

The issue is not with property type, if you look closely before the change the statements are similar, I will look who did that before and ping him if he is active, otherwise we should decide about this

@sebastienros
Copy link
Member

We reviewed it and the other related PR/issues and we found a better, simpler way to handle all cases for all filters. Mike will update his PR with that. It will use STJ serialization which is supported natively by these classes to read the configuration.

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants