Miscellaneous internal changes #256

alexdunnjpl · 2023-02-23T01:52:03Z

🗒️ Summary

Preliminary work undertaken as part of #252 which does not actually resolve it (since it has been deprioritized but the work is still useful).

Add internal IllegalArgumentException to invalid start and limit setter calls
Remove summaryOnly-related internal code and rework Singular/Plural API route behaviour to be more explicit

⚙️ Test Data and/or Report

One of the following should be included here:
See Github Actions tests

♻️ Related Issues

Fixes #255
Related to #252

tloubrieu-jpl

Overall I like the simplification of the code, but I would like to make sure the behavior is still what we expect/want.

Thanks Alex

tloubrieu-jpl · 2023-02-23T05:31:30Z

model/swagger.yml

@@ -1149,8 +1149,6 @@ components:
        syntax: limit=10

        behavior: maximum number of matching results returned, for pagination
-
-        note: limit=0 returns just the summary


I'll keep that note, although data is still returned with length 0, I would like that we made obvious to the users that limit=0 is the way to get the summary only.

Is that useful information to provide to the user? "If you request zero data you'll get zero data" seems pointless, and specifying that you'll get just the summary (which is actually the search metadata) seems to indicate that something nonstandard occurs. Your call, though.

@tloubrieu-jpl ☝️

@alexdunnjpl we wanted a way for the user to get the metadata for a set of results, beyond the pagination limits, without requesting them all. That is useful for a user who would like to know the set of properties which can be requested in this products.

The use case is:

user want to explore the product's of a collection (e.g. 100000s products)

the user needs to know which properties are used to describe these products

the user can apply search criteria on some of the available properties to subset the collection of products (these properties cannot be guessed by the user)

That is for step 2 that we initially defined the summaryOnly keyword, and then we decided to remove this keyword to use limit=0 instead. That being said it sounds like the summary does not provide the description for the full request results beyond pagination limits but only for the current page.

@tloubrieu-jpl if I understand you correctly, it sounds like limit=0 is supposed to behave in a different/non-standard manner, then? Specifically, to provide the union of the set of properties present in each product for the given query? (That's a non-homogeneous collection of property sets, yeah? Otherwise the user could just get it with limit=1 and extract the property set themselves)

It currently doesn't, iirc, so sounds like there's a need to make some kind of fix if that's necessary, but that almost seems like a job for a subroute like products/properties?{someQuery} rather than serving it implicitly in the case of products?{someQuery}&limit=0? This also seems like a potentially-expensive operation in the case of large query results, not that there's any getting around that if it's needed.

@alexdunnjpl I was unclear, I am not looking for a non standard management of limit=0. I believe what is rather needed to be updated is the way the summary is prepared. For the use case I described above, the summary need to be made from the products matching the query (q, keyword) but ignoring the pagination parameters (start, limit). I will make a more detailed ticket for that, because all the summary properties are not equivalent.

tloubrieu-jpl · 2023-02-23T05:34:21Z

service/src/main/java/gov/nasa/pds/api/registry/controller/URIParameters.java

 		}
+
+		this.limit = limit;
+		this.returnSingularDatum = false;


Somehow the property name is not meaningful to me. A bit too abstract maybe.

This replaces isSingular (dragged down from RequestAndResponseContext). The meaning is "this URIParameters is being used for an API route which returns a single element (ex. /bundles/{identifier}) rather than a collection of elements (ex. /bundles/{identifier}/products)".

Open to suggestions on this as you're right - it's not great.

@tloubrieu-jpl @alexdunnjpl ☝️

@alexdunnjpl , What about something like expectSingularResponse ?

I think it's slightly better, but I'm starting to feel like if it's worth worrying about, it's worth doing properly (see the last bit of the comment in the next thread), so unless you have an objection I think that I should

make it an enum type instead of a boolean

put an explanatory comment in the enum type so that anyone who wonders/cares what exactly it refers to can jump to the explanation with a minimum of effort

Actually, how about singletonResultExpected?

Communicates that the expected singularness applies to the result (of the query), and I think that "singleton" makes the connection a little clearer because it either means "a unique instance" or "a set with cardinality 1", both of which are correct in this context.

@tloubrieu-jpl tentatively resolved by 90c4fb9 (if you agree with the new naming)

service/src/main/java/gov/nasa/pds/api/registry/model/Pds4ProductBusinessObject.java

service/src/main/java/gov/nasa/pds/api/registry/model/RequestAndResponseContext.java

…etLimit() methods and code simplifications

tloubrieu-jpl

I fixed the conflict (that I introduced through an update on main branch).
I ran the integration test and I don't see an impact, before merging that would be good to have a review from @al-niessner.

tloubrieu-jpl · 2023-03-02T20:16:54Z

@al-niessner could you review this PR ? Thanks

alexdunnjpl · 2023-03-02T20:17:56Z

@al-niessner @tloubrieu-jpl still a couple of outstanding convos (above) before finalization, FYI

service/src/main/resources/application.properties

presence of emoji breaks branch integration testing

…tExpected

alexdunnjpl · 2023-03-02T20:43:15Z

Squash when merging, with following commit message (we've lost the commit history, but at least we can retain the granular messages):

add IllegalArgumentException throws to URIParameters setStart() and setLimit() methods
excise summaryOnly-related behaviour
rework Singular/Plural status of API routes
this is more explicit than the previous approach
remove illegal character
presence of emoji breaks branch integration testing

alexdunnjpl requested review from jimmie, nutjob4life, jordanpadams, tloubrieu-jpl and ramesh-maddegoda February 23, 2023 01:52

tloubrieu-jpl reviewed Feb 23, 2023

View reviewed changes

nutjob4life approved these changes Feb 27, 2023

View reviewed changes

alexdunnjpl mentioned this pull request Feb 28, 2023

issue 260: add parsing exception handling #266

Merged

tloubrieu-jpl force-pushed the 255-misc-changes branch from 0d41106 to c0fa24b Compare March 2, 2023 19:50

add IllegalArgumentException throws to URIParameters setStart() and s…

07e6c30

…etLimit() methods and code simplifications

tloubrieu-jpl force-pushed the 255-misc-changes branch from c0fa24b to 07e6c30 Compare March 2, 2023 20:10

tloubrieu-jpl requested a review from al-niessner March 2, 2023 20:11

tloubrieu-jpl approved these changes Mar 2, 2023

View reviewed changes

alexdunnjpl commented Mar 2, 2023

View reviewed changes

service/src/main/resources/application.properties Outdated Show resolved Hide resolved

alexdunnjpl added 2 commits March 2, 2023 12:24

revert undesirable change to default config

6b37541

remove illegal character

ce237d3

presence of emoji breaks branch integration testing

al-niessner approved these changes Mar 2, 2023

View reviewed changes

tentative rename LidvidsContext.returnSingularDatum -> singletonResul…

90c4fb9

…tExpected

alexdunnjpl merged commit 99d25e1 into main Mar 2, 2023

alexdunnjpl deleted the 255-misc-changes branch March 2, 2023 20:55

tloubrieu-jpl mentioned this pull request Mar 2, 2023

Review the summary object content #267

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous internal changes #256

Miscellaneous internal changes #256

alexdunnjpl commented Feb 23, 2023

tloubrieu-jpl left a comment

tloubrieu-jpl Feb 23, 2023

alexdunnjpl Feb 23, 2023

jordanpadams Feb 27, 2023

tloubrieu-jpl Mar 2, 2023

alexdunnjpl Mar 2, 2023 •

edited

Loading

tloubrieu-jpl Mar 2, 2023

tloubrieu-jpl Feb 23, 2023

alexdunnjpl Feb 23, 2023 •

edited

Loading

jordanpadams Feb 27, 2023

tloubrieu-jpl Mar 2, 2023

alexdunnjpl Mar 2, 2023 •

edited

Loading

alexdunnjpl Mar 2, 2023 •

edited

Loading

alexdunnjpl Mar 2, 2023

tloubrieu-jpl left a comment

tloubrieu-jpl commented Mar 2, 2023

alexdunnjpl commented Mar 2, 2023

alexdunnjpl commented Mar 2, 2023

Miscellaneous internal changes #256

Miscellaneous internal changes #256

Conversation

alexdunnjpl commented Feb 23, 2023

🗒️ Summary

⚙️ Test Data and/or Report

♻️ Related Issues

tloubrieu-jpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdunnjpl Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdunnjpl Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexdunnjpl Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

alexdunnjpl Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tloubrieu-jpl left a comment

Choose a reason for hiding this comment

tloubrieu-jpl commented Mar 2, 2023

alexdunnjpl commented Mar 2, 2023

alexdunnjpl commented Mar 2, 2023

alexdunnjpl Mar 2, 2023 •

edited

Loading

alexdunnjpl Feb 23, 2023 •

edited

Loading

alexdunnjpl Mar 2, 2023 •

edited

Loading

alexdunnjpl Mar 2, 2023 •

edited

Loading