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

text/csv format is impacted by the repairkit script (apparently) #375

Closed
tloubrieu-jpl opened this issue Aug 31, 2023 · 6 comments · Fixed by #380
Closed

text/csv format is impacted by the repairkit script (apparently) #375

tloubrieu-jpl opened this issue Aug 31, 2023 · 6 comments · Fixed by #380
Assignees
Labels
B14.0 bug Something isn't working s.high High severity sprint-backlog

Comments

@tloubrieu-jpl
Copy link
Member

Checked for duplicates

Yes - I've already checked

🐛 Describe the bug

When I do a text/csv request on "ops:Label_File_Info.ops:file_ref"

curl 'https://pds.nasa.gov/api/search/1//products?limit=10&fields=lidvid&fields=title&fields=ops%3ALabel_File_Info.ops%3Afile_ref'  --header 'Accept:text/csv'

I am getting string results with square brakets around the URL,

See example response:
"""
lidvid,ops:Label_File_Info.ops:file_ref,title
"urn:nasa:pds:epoxi_mri:hartley2_photometry:aper_phot::1.0","[https://pds-smallbodies.astro.umd.edu/holdings/pds4-epoxi_mri-v1.0/hartley2_photometry/data/aper_phot.xml]","Small Aperture Photometry"
"urn:nasa:pds:epoxi_mri:hartley2_photometry:profile::1.0","[https://pds-smallbodies.astro.umd.edu/holdings/pds4-epoxi_mri-v1.0/hartley2_photometry/data/profile.xml]","Surface Brightness Profiles"
"urn:nasa:pds:epoxi_mri:hartley2_photometry:hartley2_mri_anomaly::1.0","[https://pds-smallbodies.astro.umd.edu/holdings/pds4-epoxi_mri-v1.0/hartley2_photometry/document/hartley2_mri_anomaly.xml]","Anomalous behavior of the MRI during EPOXI approach to 103P/Hartley 2"
"urn:nasa:pds:compil-comet:nuc_properties:extinct::1.0","[https://pds-smallbodies.astro.umd.edu/holdings/pds4-compil-comet-v1.0/nuc_properties/data/extinct.xml]","Physical Properties of Extinct Comets"
"urn:nasa:pds:gbo-ctio:cfccd-19p:description::1.0","[https://pds-smallbodies.astro.umd.edu/holdings/pds4-gbo-ctio-v1.0/cfccd-19p/description.xml]","CTIO Images of 19P/Borrelly with Photometry Overview"
"""

🕵️ Expected behavior

I expected not to have the brackets inside the double quotes but outside.

📜 To Reproduce

Run a request with Accept header = text/csv

Have fields=ops:Label_File_Info.ops:file_ref

That might happen on other fields but I did not check.

🖥 Environment Info

Test on the product PDS API

📚 Version of Software Used

1.2

🩺 Test Data / Additional context

No response

🦄 Related requirements

🦄 #xyz

⚙️ Engineering Details

No response

@tloubrieu-jpl tloubrieu-jpl added bug Something isn't working s.high High severity sprint-backlog labels Aug 31, 2023
@alexdunnjpl
Copy link
Contributor

@tloubrieu-jpl there might be some confusion about correct behaviour here.

The double-quotes aren't there because they're enclosing string-typed values (as would be the case in json like the following)

["someStr", "someOtherStr"]

Rather, they're there to wrap the CSV field value, like the second line of the following:

field1, field2
"value1", "value2"

So the behaviour isn't incorrect, prima facie - whatever the desired behaviour is, it isn't that the url field values should be like ["someUrl"]


The newly-present square brackets are present because the value of the field was a string, and now it's an array containing a string.

I don't see a viable way for the serializer to determine which fields are "single values wrapped in an array because everything is wrapped in an array now" vs "a collection which happens to have only one element in it", so getting rid of the square brackets automatically for the former case isn't really a possibility.

So I guess the question is "when serializing as csv, what is the intended behaviour for fields having array input types, and does that intent change depending on the types of the elements in the array (probably not)?"

@tloubrieu-jpl
Copy link
Member Author

Thanks @alexdunnjpl , I think we need to find a better way to expose these values in the CSV because that would not make a local of sense to the users. We can discuss that at a next breakout.

@jordanpadams jordanpadams changed the title text/csv format is impacted by the repairkit script (apprently) text/csv format is impacted by the repairkit script (apparently) Sep 5, 2023
@tloubrieu-jpl
Copy link
Member Author

We chose to use the | as an internal delimiter for the multi-valued fields. We don't whant square brackets anymore around the list.

@alexdunnjpl
Copy link
Contributor

alexdunnjpl commented Sep 26, 2023

Serialization root is this call to String.valueOf()

WyriwygBusinessObject is used by virtue of being returned by this call to this.formatters.get(this.format)

~It is unclear what about WyriwygBusinessObject is specific to text/csv representation, or what else may rely upon it and its existing behavior, but ~it is not used in the default/json serialization process, which instead uses PdsProductBusinessObject.

Need to confirm that there will be no undesirable side-effects from modifying the behaviour of WyriwygBusinessObject

@tloubrieu-jpl I've tracked dependency on that serializer to application/csv, application/kvp+json, and text/csv. Modification of WyriwygBusinessObject will affect all three of these formats - is that acceptable? If not, I'll subclass it and use it just for text/csv (and application/csv, I assume?)

@tloubrieu-jpl
Copy link
Member Author

Hi @alexdunnjpl ,

Yes, I noted that application/kvp+json was also negatively impacted by the sweepers/repairkit change, so that is fine if your update fixes also this format.

alexdunnjpl added a commit that referenced this issue Sep 26, 2023
…limited format

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
alexdunnjpl added a commit that referenced this issue Sep 26, 2023
…limited format

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
alexdunnjpl added a commit that referenced this issue Sep 26, 2023
…limited format

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
alexdunnjpl added a commit that referenced this issue Sep 26, 2023
…limited format

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
@jordanpadams
Copy link
Member

Status: PR Pending

alexdunnjpl added a commit that referenced this issue Oct 9, 2023
…limited format (#380)

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
alexdunnjpl added a commit that referenced this issue Oct 10, 2023
…limited format

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
alexdunnjpl added a commit that referenced this issue Oct 10, 2023
…limited format (#382)

affects application/csv, application/kvp+json, and text/csv return types
fixes #375
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B14.0 bug Something isn't working s.high High severity sprint-backlog
Projects
None yet
3 participants