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

Support POST for /node/full/ #587

Closed
wants to merge 1 commit into from
Closed

Conversation

AbbyGi
Copy link
Contributor

@AbbyGi AbbyGi commented Oct 24, 2023

Closes #579.

@AbbyGi
Copy link
Contributor Author

AbbyGi commented Oct 24, 2023

This isn't working yet. Running the test_search.py tests locally results in the following error:
tiled.client.utils.ClientError: 405: Method Not Allowed http://local-tiled-app/api/v1/search/

@padraic-shafer
Copy link
Contributor

padraic-shafer commented Oct 24, 2023

Here is a hint...

The failing test is receiving a response that indicates only the GET method is allowed for this route.

response.__dict__ = {'status_code': 405, 'headers': Headers({'allow': 'GET', 'content-length': '31', 'content-type': 'application/json', ..., '_request': <Request('POST', 'http://local-tiled-app/api/v1/search/')>, ...

It seems that either this section (below) should revert to a GET request...or the /search/ route (and presumably others) need to have a corresponding POST route.

https://github.com/AbbyGi/tiled/blob/fee5ee77a788eaa346abeda6a86fd6c43d73a3ec/tiled/client/container.py#L251-L258

I would tend to suggest reverting to the GET version in the client; then instead write a test that verifies the new POST route in the server/router for /node/full/{path}.

Subsequently we might still need to add POST routes corresponding to all the GET data routes; similar to /awkward/buffers/{path} and illustrated by /array/*/{path} in #579.

EDIT: Corrected "array" to "xarray".

@padraic-shafer
Copy link
Contributor

With fresh eyes on this, I think it would make more sense to start from the bottom up. I believe the general /node/full/{path} POST will fail if the POST /full/{path} is not already implemented for the other structure families and media types.

E.g., Add a test to test_xarray.py that verifies a new (yet to be added) POST call. Then create the POST route that satisfies the test (without breaking other tests). Repeat this for test_array.py, test_dataframe.py, test_dataframe.py, test_hdf5.py, test_sparse.py, test_structured_array.py, test_tiff.py, and test_container_files.py. By that point, a POST route for /node/full/{path}has probably been created. If not, a container-specific test might be warranted.

I welcome input from @danielballan and others on the approach here.

@danielballan
Copy link
Member

I wonder if we should merge this first, which deprecates /node/full and may make things more clear. #612

@padraic-shafer
Copy link
Contributor

I wonder if we should merge this first, which deprecates /node/full and may make things more clear. #612

Yes, this would indeed be helpful. Thanks for putting that forward!

@padraic-shafer
Copy link
Contributor

@AbbyGi With the recent helpful updates from #612, I think we should abandon this PR and start anew. I'll move some of this discussion above to issue #579.

@danielballan
Copy link
Member

Superseded by #657

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.

Add POST option for long GET URL queries
3 participants