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

OSTI Label Extraction/LIDVID Endpoints #120

Closed
wants to merge 10 commits into from

Conversation

collinss-jpl
Copy link
Contributor

Summary
This PR adds the capability for the endpoints that retrieve an OSTI label XML file from the transaction history to extract only the appropriate record, based on matching LIDVID. This functionality has been integrated into the "release" endpoint, as well as the GET /dois/{lidvid} endpoint, which is also added by this PR. Finally, the PUT /dois/{lidvid} endpoint has been marked as "Not Implemented", in favor of the GET /dois endpoint.

Test Data and/or Report
Unit tests have been added for the /dois/{lidvid} endpoints. Test data has also been modified to better exercise the OSTI record extraction functionality.
test_results.txt

Related Issues
Reference related issues here and use Fixes or Resolves for closing issues:
#101, #105, #116

Scott Collins added 5 commits November 17, 2020 10:38
A DOI prefix/suffix is typically not available for draft records, whereas there should always be a lidvid availble to query
The function extracts a OSTI record from a listing of XML records that matches the given LIDVID.
Use of this funciton has been integrated into the release endpoint to ensure only the requested LIDVID is released.
Resolves #116
The GET endpoint retrieves a single DOI transaction record for the given LIDVID.
The PUT endpoint is marked as Not Implemented, in favor of the GET /dois endpoint.
Resolves #101
The output.xml used with transaction history tests has been modified to contain multiple records in order to test the _get_record_for_lidvid() function
@jordanpadams
Copy link
Member

@tloubrieu-jpl is this ready for merge?

@tloubrieu-jpl
Copy link
Member

I will review it today. @collinss-jpl is on vacation this week.

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

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

Thanks Scott.

I was not really able to test. I've got 404 errors and the Get /dois return 500 error on my database (see #121 )

@@ -172,31 +198,22 @@ paths:
"500":
description: Internal error
x-openapi-router-controller: pds_doi_service.api.controllers.dois_controller
/dois/{doi_prefix}/{doi_suffix}:
/dois/{lidvid}:
Copy link
Member

Choose a reason for hiding this comment

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

I was not able to make this request work, with following example:
curl --location --request GET 'http://localhost:8080/PDS_APIs/pds_doi_api/0.1/dois/urn:nasa:pds:lab_shocked_feldspars1::1.0'

Where lidvid is in my local database.

I sends a 404 error

@@ -172,31 +198,22 @@ paths:
"500":
description: Internal error
x-openapi-router-controller: pds_doi_service.api.controllers.dois_controller
/dois/{doi_prefix}/{doi_suffix}:
/dois/{lidvid}:
get:
Copy link
Member

Choose a reason for hiding this comment

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

For the put request, it sends a 404 error.
curl --location --request PUT 'http://localhost:8080/PDS_APIs/pds_doi_api/0.1/dois/urn:nasa:pds:insight_cameras::6.0'

I don't see the message that we should use POST instead.

Scott Collins added 3 commits December 1, 2020 12:03
…t define a vid were not handled correctly

A unit test has been added to test GET /dois endpoint for an entry with an LID only.
Fixes #121
…as not provided to the list action correctly

A unit test has been added to query the endpoint with just an LID.
…oint did not always result in a 501 Not Implemented response

The query parameters to the endpoint are no longer required, so sending any combination of them will now result in the expected 501 response.
A unit test has been added to demonstrate.
@collinss-jpl
Copy link
Contributor Author

@tloubrieu-jpl I've just pushed a couple commits that I think should resolve the issues you ran into, namely:

  • GET /dois endpoint should now handle entries with no VID correctly
  • GET /dois/{lidvid} endpoint should now properly handle just an LID, in addition to LIDVID
  • PUT /dois/{lidvid} should now properly respond with Not Implemented response

Let me know how things go on your end

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

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

Ok, thanks Scott, I approved the pull request and merged.

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.

None yet

3 participants