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

Add additional lidvid classes #236

Merged
merged 15 commits into from
Jan 31, 2023
Merged

Add additional lidvid classes #236

merged 15 commits into from
Jan 31, 2023

Conversation

alexdunnjpl
Copy link
Contributor

@alexdunnjpl alexdunnjpl commented Jan 19, 2023

🗒️ Summary

Implements a PdsProductIdentifier parent class of PdsLid and PdsLidVid and sinks its teeth into the codebase (i.e. changes interfaces of a large quantity of functions to use the new classes instead of Strings). This relates to #234 but is much bigger than a simple fix for that one ticket.

I don't expect anyone to read through the bulk of it, but I'd like to get feedback on the implementation of the PdsProductIdentifier class, and the general position that LID/LIDVID strings should be converted to/from PdsProductIdentifier objects as close as possible to the API's/Java's interface with external services.

If anyone does hate themselves enough to look closely, bear in mind that it's a work in progress and the expansion of mandatory PdsProductIdentifier-ness necessitates a bunch of temporary code at the incrementally-expanding interfaces (hence all the streaming collections back and forth between Strings and PdsProductIdentifiers).

The job's only half-done, but better that I get feedback now than after spending a bunch more time if there are problems with the general approach.

Resolves #234

Copy link
Member

@nutjob4life nutjob4life left a comment

Choose a reason for hiding this comment

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

Modern! This looks fine to me. Do we want hashCode?

@jordanpadams jordanpadams changed the title Moar lidvid classes - DRAFT PR FOR CODE REVIEW Add additional lidvid classes - DRAFT PR FOR CODE REVIEW Jan 20, 2023
@alexdunnjpl
Copy link
Contributor Author

alexdunnjpl commented Jan 24, 2023

Breaks (at least) http://localhost:8080/classes/collections/ endpoint, as-is.

Fixed in 992aa43

also renames URIParameters member lidvid -> productIdentifier
@alexdunnjpl
Copy link
Contributor Author

alexdunnjpl commented Jan 24, 2023

@al-niessner @tloubrieu-jpl @nutjob4life @ramesh-maddegoda

Can I get a second pair of eyes on this logging commit? It's as not-garbage as I can think to make it but maybe there's an obviously better way.

Copy link
Member

@nutjob4life nutjob4life left a comment

Choose a reason for hiding this comment

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

Sure 👍

@jordanpadams jordanpadams changed the title Add additional lidvid classes - DRAFT PR FOR CODE REVIEW Add additional lidvid classes Jan 25, 2023
@jordanpadams
Copy link
Member

@alexdunnjpl can we update this PR Summary with the ticket this is related to?

@alexdunnjpl
Copy link
Contributor Author

@jordanpadams done

@alexdunnjpl
Copy link
Contributor Author

alexdunnjpl commented Jan 31, 2023

@al-niessner courtesy ping so you don't have to look for the PR. Just the last 3 commits.

resolve() caseLATEST now has an explanatory comment (which I hope is accurate), and behaves identically to case SPECIFIC (which I just realised I should change back to case TYPED - will do that shortly).

Existing RequestConstructionContext's getLIDVID() is changed to getProductIdentifierString(), and the identifier member is swapped out for a PdsProductIdentifier in the implementation internals.

This allows for correction of RequestAndResponseContext.justLatest() to respect the fact that if a full LIDVID is the product identifier associated with the request, then the query should not filter to only the latest products because the version is specified (and may not be a latest-version product)

@alexdunnjpl alexdunnjpl merged commit 02b2ba9 into main Jan 31, 2023
@alexdunnjpl alexdunnjpl deleted the moar-lidvid-classes branch January 31, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

registry-api does not respect VID when a LIDVID is used as an id, instead returns latest version
4 participants