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-3388 Search 6 groundwork - Output the full property key (as provided by the user) in case of parsing error #1784

Closed
wants to merge 3 commits into from

Conversation

yrodiere
Copy link
Member

Copy link

@sonarcloud sonarcloud bot left a comment

Choose a reason for hiding this comment

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

SonarQube analysis found issues:
Bug Bugs: 0
Vulnerability Vulnerabilities: 0
Code Smell Code Smells: 2

See all issues in SonarCloud

@coveralls
Copy link

coveralls commented Oct 31, 2018

Pull Request Test Coverage Report for Build 2

  • 70 of 83 (84.34%) changed or added relevant lines in 16 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 86.625%

Changes Missing Coverage Covered Lines Changed/Added Lines %
engine/src/main/java/org/hibernate/search/engine/cfg/impl/FallbackConfigurationPropertySource.java 0 1 0.0%
engine/src/main/java/org/hibernate/search/engine/cfg/impl/PropertiesConfigurationPropertySource.java 0 1 0.0%
engine/src/main/java/org/hibernate/search/engine/cfg/impl/FunctionConfigurationProperty.java 8 10 80.0%
engine/src/main/java/org/hibernate/search/engine/cfg/impl/PrefixedConfigurationPropertySource.java 0 3 0.0%
engine/src/main/java/org/hibernate/search/engine/cfg/impl/OverriddenConfigurationPropertySource.java 9 15 60.0%
Files with Coverage Reduction New Missed Lines %
engine/src/main/java/org/hibernate/search/engine/cfg/impl/FallbackConfigurationPropertySource.java 1 46.67%
engine/src/main/java/org/hibernate/search/engine/cfg/impl/MaskedConfigurationPropertySource.java 1 53.33%
Totals Coverage Status
Change from base Build 46: -0.07%
Covered Lines: 11347
Relevant Lines: 13099

💛 - Coveralls

… to HibernateOrmConfigurationPropertySource

It's a bit cleaner, but more importantly that will make the following
changes easier to deal with.
Copy link

@sonarcloud sonarcloud bot left a comment

Choose a reason for hiding this comment

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

SonarQube analysis found issues:
Bug Bugs: 0
Vulnerability Vulnerabilities: 0
Code Smell Code Smells: 1

See all issues in SonarCloud

@gsmet
Copy link
Member

gsmet commented Nov 2, 2018

Can't say I'm excited by this one. Wouldn't it be better to keep the original property handy so that we don't have to resolve it back?

@yrodiere
Copy link
Member Author

yrodiere commented Nov 2, 2018

Wouldn't it be better to keep the original property handy so that we don't have to resolve it back?

Not sure what you're suggesting, but one problem is that we don't know the full property key where we use it, we only know the radical. So we can't really display the full property key without some additional code to resolve it.

For example in the ORM integration we use the hibernate.search. prefix, but in Infinispan I know for a fact that they currently (with Search 5) prepend the prefix in their integration, and users don't use it when configuring (or they use another prefix, I'm not sure). So if you consider the property defining the backend to use for "myIndex", the full key will be "hibernate.search.index.myIndex.backend" in the ORM integration, but it will be something like "index.myIndex.backend" in Infinispan. Or maybe it will be even more different, because Infinispan is configured per-cache and has only one index per cache; but in any case, this PR will allow their configuration property source to resolve the key to a string that makes sense for end users.

Out of curiosity, what is your main concern? Maybe I can do something about it.

@gsmet
Copy link
Member

gsmet commented Nov 2, 2018

Out of curiosity, what is your main concern? Maybe I can do something about it.

Well, it's just that it feels weird to have the property to begin with, then lose some information about it, and then resolve it back to what it was.

But I suppose it's how it is.

Merging.

@gsmet
Copy link
Member

gsmet commented Nov 2, 2018

Merged!

@gsmet gsmet closed this Nov 2, 2018
@yrodiere yrodiere deleted the HSEARCH-3388 branch November 16, 2018 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants