BREAKING: Make AbstractAnalysisFactory.Get non-virtual to avoid issues in constructors #800
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
laimis
changed the title
NightOwl888
approved these changes
Apr 6, 2023
There was a problem hiding this comment.
Looks good.
I think this was an intentional design decision, however, I suspect the abstract factories only exist to support SOLR and some Lucene tests. They don't really have much utility in .NET because of the odd constructors and weird way they need to be wired up with string dictionaries.
laimis
deleted the
fix/virtual-calls-from-constructors-abstractanalysisfactory
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Continuation of fixes with virtual calls being made from constructors. The issue originally reported by SonarCloud scans: https://sonarcloud.io/project/issues?resolved=false&rules=csharpsquid%3AS1699&id=apache_lucenenet and referenced in this issue: #670
In this case, AbstractAnalysisFactory calls "Get" from the constructor. Get is virtual, but it does not feel like the intent here is to make that method extendable. It's a WIDELY extended class inside Lucene via TokenFilterFactory and all the classes that inherit from it, and none of them override Get implementations.