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

HSEARCH-1383 Ability to disable automatic indexing programmatically at the session level / HSEARCH-168 Ability to disable automatic indexing programmatically at the application level #3462

Merged
merged 12 commits into from May 9, 2023

Conversation

marko-bekhta
Copy link
Member

@marko-bekhta marko-bekhta commented Mar 31, 2023

https://hibernate.atlassian.net/browse/HSEARCH-1383
https://hibernate.atlassian.net/browse/HSEARCH-168

The filter is applied in the process method for indexing the plan for this first iteration, and then there's also usage of it in the event listener to reduce the amount of work.

@marko-bekhta marko-bekhta changed the title HSEARCH-1383 Ability to disable automatic indexing programmatically at the session level HSEARCH-1383 Ability to disable automatic indexing programmatically at the session level / HSEARCH-168 Ability to disable automatic indexing programmatically at the application level Mar 31, 2023
@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch 2 times, most recently from b431e14 to f7222bc Compare March 31, 2023 18:28
@marko-bekhta
Copy link
Member Author

So the idea of filtering inside the ORM event listener somewhat failed 🥲. Since we attach the listener to the session factory, it behaves more like an application filter rather than a session filter.
I suppose we can add checks to various add/update/delete methods of AbstractPojoTypeIndexingPlan to prevent from building up the state....

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch from f7222bc to dc5b9c0 Compare April 3, 2023 07:47
@yrodiere
Copy link
Member

yrodiere commented Apr 3, 2023

So the idea of filtering inside the ORM event listener somewhat failed

It can work, but you need the listener to delegate those checks to something that's session-bound.

I gave it some thought, and I think the best approach would be for you to pass the type context to HibernateSearchEventListener#getCurrentIndexingPlan/#getCurrentIndexingPlanIfExisting and have those methods return some type-specific indexing plan.

I created a branch to show what I mean, feel free to take those commits in your own branch and adapt them as necessary: main...yrodiere:HSEARCH-1383

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch 4 times, most recently from 2ba5aa6 to ff9c395 Compare April 6, 2023 17:23
@marko-bekhta
Copy link
Member Author

Hopefully, it is a tad bit better now 🙈
by the way, I haven't used your suggestion on how to check if a type is an entity, as it didn't work as a check for a mapped super type, and I thought we wanted to give the user an option to pass these supertypes to this filter (see HibernateOrmTypeContextContainer.java#isManagedType() I'm not sure if there's a better way to check it, but if not maybe that's something to add to ORM?).

@yrodiere
Copy link
Member

I thought we wanted to give the user an option to pass these supertypes to this filter

I don't think we do. I believe mapped superclasses can be interfaces, and that opens a whole can of worms due to multiple inheritance: what if a class extends two mapped "superinterfaces", one being included and the other being excluded?

Better keep it simple, IMO.

@marko-bekhta
Copy link
Member Author

ohhh OK 🙈, then some changes are in order 😄 give me a moment 😃

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch 3 times, most recently from 9e45282 to 5149e59 Compare April 13, 2023 10:47
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Thanks! Here are a few comments, finally. Sorry for the delay.

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch 5 times, most recently from bacb0e1 to cc258ad Compare April 28, 2023 11:35
@yrodiere yrodiere self-requested a review May 1, 2023 09:41
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Thanks, it's looking better but I would advise a few more adjustments. You might want to proof-read as well, as it seems some renamings were done only partially.

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch 2 times, most recently from 2e19e0d to 8d2a975 Compare May 2, 2023 20:46
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

LGTM apart from two minor comments. Feel free to merge once they're addressed.
Thanks!

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch from 8d2a975 to 3ca6420 Compare May 3, 2023 15:43
@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch from 3ca6420 to a11a7d0 Compare May 3, 2023 16:27
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, though I think I spotted an unnecessary check. Please have a look and then feel free to merge!

@marko-bekhta marko-bekhta force-pushed the feat/HSEARCH-168-index-filter branch from a11a7d0 to 765681e Compare May 9, 2023 09:58
@sonarcloud
Copy link

sonarcloud bot commented May 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 17 Code Smells

84.5% 84.5% Coverage
0.0% 0.0% Duplication

@yrodiere
Copy link
Member

yrodiere commented May 9, 2023

Merged, thanks!

@yrodiere
Copy link
Member

yrodiere commented May 9, 2023

Actually, not merged, GitHub is having problems...

@yrodiere yrodiere merged commit dfe92c1 into hibernate:main May 9, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants