podaac / hydrocron

API for retreiving time series of SWOT data
https://podaac.github.io/hydrocron/
Apache License 2.0
16 stars 4 forks source link

When a user specifies that they want unit fields, return is empty. #184

Open cassienickles opened 4 months ago

cassienickles commented 4 months ago

When a user specifies fields = "reach_id,time,time_units,time_tai,time_tai_units,time_str,p_lat,wse" it returns empty. Upon investigation, it only returns empty when the unit fields are added (time_units, time_tai_units, etc). If those fields are left out from being specified, hydrocron works as expected and the units fields are appended in the results. (ie. fields = "reach_id,time,time_tai,time_str,p_lat,wse" ) Is this an error in the code somewhere?

torimcd commented 4 months ago

The error that gets returned is "400: fields parameter should contain valid SWOT fields"

Looks like the code that adds units may be adding "_units" again when the units are in the requested fields, eg resulting in invalid fields like time_units_units: https://github.com/podaac/hydrocron/blob/ab4ffb74389c37fbf99e4beb6c8493847ffa3a4d/hydrocron/api/controllers/timeseries.py#L329C1-L340C34

Might need to add a check to skip this for fields where units are already specified, but this gets complicated in the example above when some units fields are explicitly requested and other fields that have units are not, like wse.

@cassienickles what behavior do you expect in the example you posted? Would you expect the units to also be returned for the wse field even though it's not explicitly requested?

cassienickles commented 4 months ago

I'm wondering that since we automatically add the units if a parameter has units, maybe we don't make it known in the documentation that the '_units' fields can be requested and instead let them know that the '_units' fields will just automatically be returned if they are available.

So in this documentation (https://podaac.github.io/hydrocron/timeseries.html#request-parameters), remove the '_units' parameters from fields that can be requested and add a blurb that unit fields will be returned as well? Thoughts?

nikki-t commented 4 months ago

What I think is happening is the API code will look at the constants.REACH_ALL_COLUMNS and constants.NODE_ALL_COLUMNS to determine if the user passed in a valid list of fields in their request. See code here.

Since the units are not specified in the reach and node column lists, the API code returns an invalid request response.

The way that the API code is written now, is if we add units to those lists then the API code will try to add units to the units fields as mentioned by @torimcd.

@cassienickles - I like your idea of removing the _unit fields from the documentation since we always return the units if they are available and we can avoid complicated conditional statements to try to accommodate various requests for different units. Happy to change things in the future if the need arises!

For now, I will update the documentation since I also need to spend some time with the documentation in issue #183.

torimcd commented 4 months ago

I think updating the documentation is fine for the immediate issue, but I think we should be able to have the API handle this use case gracefully in the future. We want people to be using units, so once they see them being returned it makes sense that they will request them in future API calls.

Ideally, we would also have the units listed in the fields that can be returned. We should think about how to do this in line with #112 as well

frankinspace commented 4 months ago

👍 handle it in documentation for now