Pds api 110: unify the behavior of response building #80

al-niessner · 2021-11-01T22:17:49Z

🗒️ Summary

Brief summary of changes if not sufficiently described by commit messages.

⚙️ Test Data and/or Report

One of the following should be included here:

Reference to regression test included in code (preferred wherever reasonable)
Attach test data here + outputs of tests

♻️ Related Issues

Extend application/pds4+json support to all endpoints pds-api#110

Basically scrapped what was there and reqbuilt it all using a visitor pattern that will hopefully consolidate changes that fix all endpoints instead of so much cut-n-past. There is still a lot of boilerplate thata may be able to be worked out with more effort.

al-niessner · 2021-11-01T22:18:56Z

@tloubrieu-jpl @jordanpadams @tdddblog

Its finally in place. Needs testing which I will get to Wednesday probably. Let me know if you want to review it in the breakout tomorrow.

Had to add "accept: application/json to the verify scripts. This moved them formward but have some odd results like the query language changed without updating the test. Other items are still failing and will need to track them down one by one but the problems are no longer in the verify scripts. Added some missed functinality in the product handler. Cleaned up some logic in the RequestAndResponseContext that does the bulk of the work for mixing and matching the various application type requests. Added a new error to account for when valid information was given (lidvid) but nothing was found despite that.

The looking up a lidvid from lid required a touch more fine tuning to verify that it found the whole lidvid matches the given to accound for partial lidvids that are more than a lid but less than a full lidvid. Had to also clean up a List<String>.toArray() that was causing a segfault. Turning a list to [] syntax is not so easy to understand.

al-niessner · 2021-11-08T19:00:37Z

@tloubrieu-jpl @tdddblog @jordanpadams

All the tests, not query language related, work now. You can get either PDS-4 (pds4+json) format or our favorite format (json). You can add new output styles simply by adding to the table RequestAndResponseContext.formatters.

The work of making the product and adding them all to the endpoints. THe work of adding a serializer is a touch more complicated.

tloubrieu-jpl

Hi @al-niessner ,

I am getting null pointer exception with the following requests:

curl --location --request GET 'http://localhost:8080/collections?limit=10'
--header 'Accept: application/json'

and

curl --location --request GET 'http://localhost:8080/products?only-summary=True'
--header 'Accept: application/json'

I am using pds-api-javalib branch swaggerhub-pds_federated_api-pds-api-110-4

jordanpadams

@al-niessner in the API spec, shouldn't this:

        application/csv+text:
           schema:
            $ref: '#/components/schemas/wyriwygProduct'

be this?

        text/csv:
           schema:
            $ref: '#/components/schemas/wyriwygProduct'

also I'm guessing wyriwygProduct is just a placeholder for those other inwork response formats?

al-niessner · 2021-11-12T19:39:13Z

Can change application/cvs+text to text/cvs at any time. WYRIWIG - What You Request Is What You Get. Stolen from WYSIWIG editors. If you want a better name, make it up and it can be changed.

…

On Fri, Nov 12, 2021 at 10:39 AM Jordan Padams ***@***.***> wrote: ***@***.**** commented on this pull request. @al-niessner <https://github.com/al-niessner> in the API spec, shouldn't this: application/csv+text: schema: $ref: '#/components/schemas/wyriwygProduct' be this? text/csv: schema: $ref: '#/components/schemas/wyriwygProduct' also I'm guessing wyriwygProduct is just a placeholder for those other inwork response formats? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIUBIVRGODL46IILQDA4SLULVNOFANCNFSM5HFBKXPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

al-niessner · 2021-11-12T19:40:29Z

That is the right javalib branch. I do not get a null pointer exception.

…

On Thu, Nov 11, 2021 at 10:48 AM thomas loubrieu ***@***.***> wrote: ***@***.**** commented on this pull request. Hi Al, I am getting null pointer exception with the following requests: curl --location --request GET 'http://localhost:8080/collections?limit=10' --header 'Accept: application/json' and curl --location --request GET ' http://localhost:8080/products?only-summary=True' --header 'Accept: application/json' I am using pds-api-javalib branch swaggerhub-pds_federated_api-pds-api-110-4 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#80 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIUBIRV2M43VAUSH73F3RTULQFYXANCNFSM5HFBKXPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

tloubrieu-jpl · 2021-11-12T19:43:51Z

Hi @al-niessner , do you use a specific branch for the csv development ? If not you should branch off of this branch pds-api-110 to create a new branch for the CSV development so that this pds-api-110 stays stable for the tests.

Thanks

al-niessner · 2021-11-12T19:46:24Z

Sorry, forgot to create a new branch. I will do that for te webmvcc portion.

…

On Fri, Nov 12, 2021 at 11:44 AM thomas loubrieu ***@***.***> wrote: Hi @al-niessner <https://github.com/al-niessner> , do you use a specific branch for the csv development ? If not you should branch off of this branch pds-api-110 to create a new branch for the CSV development so that this pds-api-110 stays stable for the tests. Thanks — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#80 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIUBIR67RO7SLMZU7YWU4LULVVAFANCNFSM5HFBKXPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

al-niessner · 2021-11-15T19:21:40Z

Hi @al-niessner ,

I am getting null pointer exception with the following requests:

curl --location --request GET 'http://localhost:8080/collections?limit=10' --header 'Accept: application/json'

and

curl --location --request GET 'http://localhost:8080/products?only-summary=True' --header 'Accept: application/json'

I am using pds-api-javalib branch swaggerhub-pds_federated_api-pds-api-110-4

@tloubrieu-jpl

This is not due to my changes. The input coming in from the curl makes q null. The search request builder is unmodified so the problem may be inherited from the main branch.

First, I added these two log lines:

        	log.debug("the input query string: " + String.valueOf(q));
        	log.debug("the context query string: " + String.valueOf(context.getQueryString()));

and produce this result:

2021-11-15 18:32:23.258 DEBUG 54 --- [0.3-8080-exec-1] g.n.p.a.e.c.MyProductsApiBareController  : the input query string: null
2021-11-15 18:32:23.258 DEBUG 54 --- [0.3-8080-exec-1] g.n.p.a.e.c.MyProductsApiBareController  : the context query string: null

Second, I went back to the main branch to test it. It works fine because somewhere it is translating the null to "". Going to update for that and see if it works.

tloubrieu-jpl · 2021-11-15T19:24:25Z

Thanks @al-niessner

The blank query had to use the preset which got lost. Also put the conversion of the names back in place.

al-niessner · 2021-11-15T21:35:11Z

@tloubrieu-jpl

Fixed up the curl. Please try again or continue with review.

Accidentally made it so only the minimal set was returned if fields are left blank. Added a min/max list of fields where min must be a subset of max and empty is a full set without having to define all that may be in elastic search. This min/max approach fixes the blank fields and adds constraints so that pds+ formats will only every reeturn the allowable keys and nothing else. These are simple to change in the future as well by modifying just the min/max arrays.

tloubrieu-jpl

Good to go, no much time to look at the details but that worked with my tests

Al Niessner added 5 commits October 14, 2021 09:24

fix all the shortcuts to accept application/pds4+json

7bc30e2

changes to support swagger.yaml changes

4bc6ab7

middle of refactor

4ec1489

back to ResponseEntity<Object>

e0754d9

al-niessner requested a review from a team as a code owner November 1, 2021 22:17

Al Niessner added 3 commits November 4, 2021 16:04

set these to something workable to everyone but me

03fa2a1

add cvs, cvs+text,kvp+json

71ae90d

The work of making the product and adding them all to the endpoints. THe work of adding a serializer is a touch more complicated.

tloubrieu-jpl reviewed Nov 11, 2021

View reviewed changes

jordanpadams reviewed Nov 12, 2021

View reviewed changes

fixed up the query

4f4a5d1

The blank query had to use the preset which got lost. Also put the conversion of the names back in place.

al-niessner mentioned this pull request Nov 18, 2021

serializer framework to keep #84

Closed

tloubrieu-jpl approved these changes Nov 18, 2021

View reviewed changes

tloubrieu-jpl merged commit 7a9456a into main Nov 18, 2021

al-niessner mentioned this pull request Nov 22, 2021

Updates for CSV and KVP response formats #90

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pds api 110: unify the behavior of response building #80

Pds api 110: unify the behavior of response building #80

al-niessner commented Nov 1, 2021 •

edited by jordanpadams

Loading

al-niessner commented Nov 1, 2021

al-niessner commented Nov 8, 2021

tloubrieu-jpl left a comment •

edited

Loading

jordanpadams left a comment

al-niessner commented Nov 12, 2021 via email

al-niessner commented Nov 12, 2021 via email

tloubrieu-jpl commented Nov 12, 2021

al-niessner commented Nov 12, 2021 via email

al-niessner commented Nov 15, 2021

tloubrieu-jpl commented Nov 15, 2021

al-niessner commented Nov 15, 2021

tloubrieu-jpl left a comment

Pds api 110: unify the behavior of response building #80

Pds api 110: unify the behavior of response building #80

Conversation

al-niessner commented Nov 1, 2021 • edited by jordanpadams Loading

🗒️ Summary

⚙️ Test Data and/or Report

♻️ Related Issues

al-niessner commented Nov 1, 2021

al-niessner commented Nov 8, 2021

tloubrieu-jpl left a comment • edited Loading

Choose a reason for hiding this comment

jordanpadams left a comment

Choose a reason for hiding this comment

al-niessner commented Nov 12, 2021 via email

al-niessner commented Nov 12, 2021 via email

tloubrieu-jpl commented Nov 12, 2021

al-niessner commented Nov 12, 2021 via email

al-niessner commented Nov 15, 2021

tloubrieu-jpl commented Nov 15, 2021

al-niessner commented Nov 15, 2021

tloubrieu-jpl left a comment

Choose a reason for hiding this comment

al-niessner commented Nov 1, 2021 •

edited by jordanpadams

Loading

tloubrieu-jpl left a comment •

edited

Loading