Issue 131: increase adaptability #142
Merged
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
added 18 commits
May 4, 2022 14:47
Having /{group} and /{id} is too much for swagger as it is not smart enough at the Java layer to differentiate. Had to put gid (group ID) and uid (unique ID as in lid, lidvid, or doi or...) in the front to separate them out.
/gid/{group} and /uid/{identifier} are working again. Need to test the parent/child relationships but seems to be on the right track.
/ still is broken but it seems to be a spring 2.6.6 thing. Not the only person in the world to have this problem but it is not about finding / as much as finding the redirect to /swagger-ui.html. Need to track that down.
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
service/src/main/java/gov/nasa/pds/api/registry/ReferencingLogic.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/ReferencingLogic.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/controller/SwaggerJavaTransmuter.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/model/ReferencingLogicTransmuter.java
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/ReferencingLogic.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/ReferencingLogic.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/controller/SwaggerJavaTransmuter.java
Outdated
Show resolved
Hide resolved
service/src/main/java/gov/nasa/pds/api/registry/ReferencingLogic.java
Outdated
Show resolved
Hide resolved
|
Hi @al-niessner , @jordanpadams I am not finding the ticket where Al's full proposal for refactoring the URL path is (although I have pasted your proposal in a note of mine). Here is my proposal, that we can discuss this afternoon: Drivers for the design:
Search and identifier resolution: For all products: For specific groups of products (classes):
Note the list given in https://pds.nasa.gov/datastandards/documents/dd/current/PDS4_PDS_DD_1H00.html#d5e85 is sometimes not consistent in having nouns as the part after Product_ although most of them are nouns. Rest-ful apis prefers nouns (see https://restfulapi.net/resource-naming/ ). We could replace the adjective or verbs by related nouns:
in swagger specification we would only have this endpoint entry: Referencing: From any identifier resolution URL (for example http://pds.nasa.gov/api/search/1.0/observationals/{id} or http://pds.nasa.gov/api/search/1.0/bundles /{id}) we can get their references by roles of the association as shown on the diagram on page https://pds.nasa.gov/datastandards/documents/im/current/index_1H00.html For example:
Alternate option is not to have instead of collection-members only members or instead of bundle-owners only owners . That would less explicit for the user but more simple and that might be obvious enough/documented elsewhere for the users. products/{id} would not have any referencing besides the generic referencing that a product might have, could be for example Citation_Information which applies to any class of product (see https://pds.nasa.gov/datastandards/documents/dd/current/PDS4_PDS_DD_1H00.html#N-2006280352). Although we don't have specific entries with identifiers for dois in the registry yet. |
|
/products/{id}/[containers,members]/[all,latest] |
|
CANCELLED |
|
@al-niessner I am seeing that you developed regression tests in python. We would like to consolidate the postman test suite as well (which can be run also in command line), could you work with that in the future ? Thanks If we can have unit tests they would be developed in Java but I believe what you did is integration tests since they require the registry database to be up and populated. Let me know if you have questions on that. Thanks |
|
Yes, will move to postman when postman checks are integrated into checks done a pull request. |
|
Hi @al-niessner I am getting an error when I do the request ``http://localhost:8081/` which redirects on Error sent is: Logs give: Thanks |
|
Yes. It is the upgrade to latest spring that did this. Not sure how to fix but needs another ticket as it is not related to this work. Although it might be some weird resource that I am not aware of that needs updating because of name changes in this work. However when googled, it seems to be known and related to latest version of spring. See #141 |
|
Thanks @al-niessner for creating the ticket |
|
Hi @al-niessner , Weirdly, I am having this error again that we were not able to reproduce in ticket #133 The request: Returns again: The same thing happen with this request: |
|
My Python unit tests stay that those do work in the new code. Does not verify it is the right lidvid but verifies that it 1 returned and that an actual lidvid comes with it. Being that you thought release 1.0.1 had same problem I am going to say it is your test harness and/or database. Are you testing by hand or are you using postman? |
Strange JUnit thing where org.junit.Test does not work but org.junit.jupiter.Test does. Seems the antlr unit tests are back on line. Tried updating to todays latest spring to see if that fixes the root white label error problem.
It is swagger 2 that change the help. It is now /swagger-ui/index.html. All references have been updated.
added 2 commits
June 11, 2022 14:45
Merged
|
Dev review:
|
tloubrieu-jpl
approved these changes
Jun 13, 2022
There was a problem hiding this comment.
Thanks @al-niessner I successfully tested all the tickets embedded in this pull request.
I only removed #128 which does not sound like it is part of the PR (it is closed anyway).
I created 2 minor tickets:
#150 which is alike #133, hopefully you will be able to reproduce this time. I need you to help me to understand why I am having this behavior.
#149 a minor ticket on the newly repaired swagger-ui
Thanks again, you can merge this PR
Thomas
al-niessner
added a commit
that referenced
this pull request
Aug 16, 2022
* Issue 131: increase adaptability (#142) * endpoints Had to change the planned endpoints to /gid/{group name} to /pid/{lidvid}. Also had to add /gid and /pid as prefixes for the referencing endpoints as well. The point really is that had to add the prefixes is because swagger cannot tell /{group name} from /{lidvid} even though the group names are a well defined enum. The prefixes allow swagger to identify the endpoints as unique. While the actual characters gid and pid are not required, whatever is chosen must be unique and different. * reshape the architecture/design The base architecture is MVC, but the code does not directly represent it. Moved business to model and serializer to view to match the architecture. Reduced the multiple controllers to one and use a facade/plug-n-play pattern (ReferencingLogic) to manage the polymorphism needed for each of the ancestral paths to find information. Added a Java enum to do the mapping among the group names and the product class in the data itself. The enum holds the ReferencingLogic allowing either side (controller or model) to work with the given inputs and collect the desired output. Most of the code in the DAO objects moved to ReferencingLogic implementations. These objects understand the relationship between model elements and internalize the relationship as code. An example is for bundles finding their grandchildren. * CONTRIBUTING.md updated Once the refactoring was complete, updated the documentation to reflect the new code naming and its relationship to the architecture/design. * docker update by Jimmie Young <[email protected]> * Add a Dockerfile for building images for AWS deployments (#151) * Creation of AWS-specific Dockerfile, update README to indicate its purpose * Fix typo in both Dockerfiles (extraneous ']' in CMD) * Anonymous changes from main stomp * Switch back to 8080 by default (#157) * fix for 141 (#147) (#156) * changed string searching (#158) by Al Niessner <[email protected]> Seems that wildcards is not well implemented in the later elasticsearch and opensearch implementations. The most recent documentation for either has changed significantly that if searhing for strings, then they offer a short globish like set of patterns. Moved from using wildcards to this new string and it seems to be working. The eq operator had some problems because it was using should instead of filter for the or condition. SHould returns everything with a score value to represent the closes match from the furthest. Changed over to filter and the or condition is now behaving as expected. * Anonymous changes from main stomp * fixes for issue 162 * fixes for issue 162 * fix for issue 149
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.
🗒️ Summary
Changed endpoints to be more adaptable to changes
⚙️ Test Data and/or Report
Ran tests developed previously to check the basic combinations as step before integration testing:
♻️ Related Issues
#141
#133
#131
#118
#12
#11