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 NRE on migration from 1.4 -> 1.5 due to Lucene changes #12831

Merged
merged 4 commits into from Nov 19, 2022

Conversation

PBMikeW
Copy link
Contributor

@PBMikeW PBMikeW commented Nov 16, 2022

Lucene Search Migration from 1.4 to 1.5 fails due to null reference error.

@PBMikeW PBMikeW changed the title Fix NRE Fix NRE on migration from 1.4 -> 1.5 due to Lucene changes Nov 16, 2022
@Skrypt
Copy link
Contributor

Skrypt commented Nov 17, 2022

Could you change this also in the 2 other places where I did the same.

if ((bool)included && !(bool)analyzed)

@Skrypt Skrypt requested review from jtkech and Skrypt November 18, 2022 14:55
Skrypt
Skrypt previously approved these changes Nov 18, 2022
Fix like JTKech suggestion
@Skrypt
Copy link
Contributor

Skrypt commented Nov 19, 2022

Ok, I think I got this. Basically, a field cannot be analyzed and a keyword at the same time.
And previously the settings were:

Included = keyword (Set when the option "Include this element in the index" was checked)
Analyzed = tokenized (Set when the option "Analyzed" was checked)

Now, I believe that we had all the fields as "Keywords" by default which was the opposite of what ElasticSearch does. Also, it was not making any sense to have it set as a Keyword by default because most people will want these fields to be tokenized/analyzed by default because that's the goal of it ... You want to have only fewer specific fields to be set as Keyword for using them as technical values most of the time.

Based on the fact that "Analyzed" is now the option when you check "Include this element in the index".

old value new value comment
!included + analyzed included This was impossible to happen but we never know
included + analyzed included Here we were specifying that we want the field to be analyzed
included + !analyzed keyword
included keyword same as previous but with a null Analyzed value

So we need to take into consideration that the "Analyzed" option is now "Included". We don't need to migrate the "Included" to a different value just set the new "Keyword" one based on which of these fields had only "Included" set on them.

@Skrypt Skrypt dismissed their stale review November 19, 2022 03:27

I did the changes

@Skrypt Skrypt requested a review from jtkech November 19, 2022 03:28
@Skrypt Skrypt merged commit 4d326e9 into OrchardCMS:main Nov 19, 2022
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.

None yet

3 participants