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

CUSTCOM-204 - Fixed comparator of OpenApiWalker's internal Tre… #4496

Merged
merged 1 commit into from Feb 21, 2020

Conversation

@Pscheidl
Copy link

Pscheidl commented Feb 15, 2020

…eSet of allowed classes

Attempts to fix #4489

Description

This is a bug fix for #4489.

As described in JavaDoc above the test for this fix, a TreeSet has been used inside OpenApiWalker class to ensure low lookup times during the OpenApi schema creation/generation process. Which is a clever choice, as a TreeSet ensures O(log(n)) asymptotic complexity source.

The previous comparator, with it's limited number of outputs for the whole set of allowed classes with typically high cardinality, was effectively making some classes not foundable on lookup, even though those classes were present. On lookup, the comparator sent the pointer to a completely different direction/tree node.

I believe a cheap and optimal solution would be to compare distance among classes using Strings. To quote the JavaDoc above the test created in this PR:

The compared objects inside the internal comparator supplied to the Set implementation are of type "Class".
And classes have no natural distinct ordinal numbers in Java. One way to get an ordinal number for a Class is
to use it's fully qualified name and compare the distance to the other compared class in the space consisting
of other class names. This way, "more similar" strings have closer cartesian distance. And as String 
representation of a Classs is used, fast lookup is ensured.

The implementation of String.compareTo method is fast & efficient and is able to quickly send the lookup pointer the right way in the tree by using the whole space of all possible class names. The opposite would be comparing by +1/-1 steps could also mean linear search in worst case.

Important Info

Testing

New tests

OpenApiWalkerTest class with one new test named testClassOrderingAndSearch.

This class tests:

  1. All inserted classes are found afterwards in the internal TreeSet
  2. Internal contract is defined as Set
  3. The implementation used is a TreeSet - debatable. If anyone changes the implementation, this test will fail and the person changing the code will be forced to change the test and thus cinciously consider the lookup complexity.

Testing Performed

Created a JUnit test.

Testing Environment

Oracle JDK 1.8.0_241
Ubuntu 19.10

…eSet of allowed classes
@Pscheidl Pscheidl force-pushed the Pscheidl:microprofile-4489 branch from 2d628c5 to 30a2624 Feb 15, 2020
@smillidge smillidge requested review from MattGill98 and jbee Feb 16, 2020
Copy link
Member

MattGill98 left a comment

Further to my earlier comment, this looks good to me - thank you for the contribution!

@jbee

This comment has been minimized.

Copy link
Contributor

jbee commented Feb 20, 2020

jenkins test please

@MattGill98 MattGill98 changed the title MICROPROFILE #4489 - Fixed comparator of OpenApiWalker's internal Tre… CUSTCOM-204 - Fixed comparator of OpenApiWalker's internal Tre… Feb 20, 2020
@MattGill98 MattGill98 merged commit b41502e into payara:master Feb 21, 2020
58 checks passed
58 checks passed
Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
@Pscheidl Pscheidl deleted the Pscheidl:microprofile-4489 branch Feb 21, 2020
@Pscheidl

This comment has been minimized.

Copy link
Author

Pscheidl commented Feb 21, 2020

Thank you guys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.