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

CTS regression on advanced find tests? #3202

Closed
cmgrote opened this issue Jun 12, 2020 · 7 comments
Closed

CTS regression on advanced find tests? #3202

cmgrote opened this issue Jun 12, 2020 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@cmgrote
Copy link
Member

cmgrote commented Jun 12, 2020

There was a recent change to the advanced find tests to run the .matches() method against manually-constructed regular expressions that include a dynamically-determined string.

Unfortunately, this dynamically-determined string could itself contain regex-meaningful characters (for example: (, ., etc). In particular, when there is a parenthesis involved that does not have a corresponding opening / closing parenthesis, this will result in a regular expression exception; for example:

java.util.regex.PatternSyntaxException: Unclosed group near index 40
.*FOSVR::(database)=LOCATION::(databas.*
        at java.util.regex.Pattern.error(Pattern.java:1969)
        at java.util.regex.Pattern.accept(Pattern.java:1819)
        at java.util.regex.Pattern.group0(Pattern.java:2922)
        at java.util.regex.Pattern.sequence(Pattern.java:2065)
        at java.util.regex.Pattern.expr(Pattern.java:2010)
        at java.util.regex.Pattern.compile(Pattern.java:1702)
        at java.util.regex.Pattern.<init>(Pattern.java:1352)
        at java.util.regex.Pattern.compile(Pattern.java:1028)
        at java.util.regex.Pattern.matches(Pattern.java:1133)
        at java.lang.String.matches(String.java:2121)
        at org.odpi.openmetadata.conformance.tests.repository.instances.TestSupportedEntitySearch.performAdvancedSearchTests(TestSupportedEntitySearch.java:2110)
        at org.odpi.openmetadata.conformance.tests.repository.instances.TestSupportedEntitySearch.performFinds(TestSupportedEntitySearch.java:682)
        at org.odpi.openmetadata.conformance.tests.repository.instances.TestSupportedEntitySearch.run(TestSupportedEntitySearch.java:257)
        at org.odpi.openmetadata.conformance.beans.OpenMetadataTestCase.executeTest(OpenMetadataTestCase.java:414)
        at org.odpi.openmetadata.conformance.workbenches.repository.RepositoryConformanceWorkbench.runTests(RepositoryConformanceWorkbench.java:426)
        at org.odpi.openmetadata.conformance.workbenches.repository.RepositoryConformanceWorkbench.run(RepositoryConformanceWorkbench.java:556)
        at java.lang.Thread.run(Thread.java:748)

Note that strings (like qualifiedName values) could easily contain parentheses, and further if the strings are dynamically truncated there's no guarantee that both the opening and closing parenthesis will be present. (The exception above is thrown by the CTS itself, where the .match() method is called, rather than from any underlying repository or connector.)

@grahamwallis looks like you made the change, and I'm not sure what the surrounding context was for switching to the .match() calls from the prior approach, so hoping you'll have ideas on how to resolve? If we need to retain the .match(), it sounds like we need to either surround the truncated string with a literaliser (\Q and \E), or if we actually want to treat the truncated string as a regular expression then you'll probably have to perform some actual magic -- no idea how we can determine whether a randomly-truncated regular expression would itself end up being a well-formed regular expression (?)

Final note: this is present in both the 1.8 release as well as master.

@cmgrote cmgrote added the bug Something isn't working label Jun 12, 2020
@grahamwallis
Copy link
Contributor

Thanks for spotting and raising this @cmgrote.
Looks like this area needs further work. The original change was needed because performing an entirely literal match of the (truncated) string was at times yielding different results to the real regex match that a supporting repo performs. For example, the truncated search string can contain regex-significant characters - I discovered the original problem due to a '.' (dot) character in the search string. By performing only a literal match, the expected results were not inclusive enough, and this resulted in valid returned results being flagged as 'not expected' and hence causing a test failure when in fact the repo was doing exactly what the test was asking for. To address that problem I switched to using regex matches in the generation of the expected result set and ran the CTS suite on the in-mem and graph repos - which of course relies on the injected test instances, which do contain '.' (dot) characters but not other regex special characters. II hadn't anticipated the problem you mention above - that the truncation may lead to a nonsensical regex pattern - so will need to look at this again.

@planetf1
Copy link
Member

Can confirm cts ran successfully for the 1.8 release. Looks as if this is a CTS issue (at least initially) so we continue work on this in master only. Not looking as if anything needed for 1.8 (which has already completed distribution)

@grahamwallis
Copy link
Contributor

@cmgrote I think this is now fixed (by 3228). Please could you verify that it works OK with IGC?

@grahamwallis
Copy link
Contributor

I should have added that the merged PR temporarily makes the CTS output the escaped regex pattern it uses in each test. Once we are happy that the PR addresses the problem I will remove the verbose logging.

@cmgrote
Copy link
Member Author

cmgrote commented Jun 22, 2020

Looks good now, thanks!

@grahamwallis
Copy link
Contributor

Great - thanks for testing it. I will generate a further PR to turn off the logging and close this issue when that PR is merged.

@grahamwallis
Copy link
Contributor

Logging removed in PR #3240

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants