OAK-10590 - If includedPaths and excludedPaths are specified as a String instead of array of String, interpret them as a one-element array of Strings#1254
Merged
nfsantos merged 7 commits intoapache:trunkfrom Dec 21, 2023
nfsantos:OAK-10590
Merged
OAK-10590 - If includedPaths and excludedPaths are specified as a String instead of array of String, interpret them as a one-element array of Strings#1254nfsantos merged 7 commits intoapache:trunkfrom nfsantos:OAK-10590
nfsantos merged 7 commits intoapache:trunkfrom
nfsantos:OAK-10590
Conversation
…a String instead of array of String, log a warning and treat them as a one-element array instead of assuming that it they are not defined.
| return property.getValue(Type.STRINGS); | ||
| } else if (property != null && property.getType() == Type.STRING) { | ||
| String value = property.getValue(Type.STRING); | ||
| LOG.warn("Property \"{}\"=\"{}\" has type String but it should be array of String. Proceeding by treating it as a " + |
Member
There was a problem hiding this comment.
I would probably not log a warning. Instead, I would document that this is also supported.
Reason: we already have many many entries that are defined as string (mostly "/dummy") instead of array.
Member
There was a problem hiding this comment.
I would make it public and add a unit test case for it.
…as being done incorrectly, by setting the property as a string instead of array of strings.
…ept a string value as being a one-element array (NodeCounterMBeanEstimator). Other refactoring to clean up the code.
…TRING instead of STRINGS. Simplified the logic of extracting a list of strings from a property that can be STRING or STRINGS by taking advantage of the default value of PropertyState.getValue(STRINGS), which handles the case of the property being a STRING.
fabriziofortino
approved these changes
Dec 21, 2023
nfsantos
changed the title
rishabhdaim
pushed a commit
that referenced
this pull request
Jan 25, 2024
…ing instead of array of String, interpret them as a one-element array of Strings (#1254)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The includedPaths property of an index definition should be an array of strings. But a common mistake made by users is to define it as a String when it has a single element. That is, instead of:
it is defined as:
If
includedPathsis defined as a String, the indexing job would ignore its value and instead default to use the root as theincludedPaths, which results in downloading the full node store and creating an FFS containing everything (except hidden paths). This will slow down significantly the indexing job, as it will negate any benefits from using regex filtering. And even if regex filtering is not enabled or cannot be used, using/asincludedPathswill also result in the FFS containing more nodes than it should, which will once again slow down the indexing job.The same is true for
excludedPaths, but in this case, the default value is an empty list ofexcludedPaths, so it will ignore the value of this property and will not exclude anything. This may result in parts of the node store being indexed that should not be indexed.The handling of
includedPathsandexcludedPathsis also inconsistent with the handling ofqueryPaths, which is being interpreted as a one-element array if it is defined as a String.This PR makes the logic that reads the
includedPathsandexcludedPathsproperties more lenient, by treating Strings as one-element arrays and issuing a warning with a suggested fix.Additionally, the PR makes some minor cleanups in the files that had to be modified (remove usages of Guava and fix some compilation warnings).