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

Collision between /documents/id endpoint and /documents/export endpoint #13

Closed
jasonbosco opened this issue Jan 4, 2018 · 8 comments
Closed
Assignees
Labels

Comments

@jasonbosco
Copy link
Member

Description

The possibility of this collision was in the back of my head when writing the API spec... Looks like it has indeed manifested itself in the implementation.

Currently, the URL to retrieve/delete a doc is /collections/{collectionName}/documents/{documentId}. So when you do a GET on /collections/{collectionName}/documents/export it says "document with ID export is not found" instead of considering /export an action endpoint.

Steps to reproduce

$ curl -X GET "http://localhost:8108/collections/companies/documents/export" -H  "accept: application/json" -H  "X-TYPESENSE-API-KEY: abcd" -sv
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 8108 (#0)
> GET /collections/companies/documents/export HTTP/1.1
> Host: localhost:8108
> User-Agent: curl/7.54.0
> accept: application/json
> X-TYPESENSE-API-KEY: abcd
>
< HTTP/1.1 404 Not Found
< Date: Thu, 04 Jan 2018 15:55:50 GMT
< Connection: close
< Access-Control-Allow-Origin: *
< content-type: application/json; charset=utf-8
<
* Closing connection 0
{"message": "Could not find a document with id: export"}

Expected Behavior

It should export all documents in the collection.

Actual Behavior

It considers export an ID and says the doucment isn't found

Metadata

Typsense Version: 0.8-api-changes

@kishorenc
Copy link
Member

Interesting. I don't like the idea of blacklisting export as something special which cannot be allowed as a valid document ID.

Can we define the export end-point as /collections/{collectionName}/export.jsonl? It indicates that you are exporting the contents of the collection as jsonl.

@jasonbosco
Copy link
Member Author

jasonbosco commented Jan 5, 2018

Can we define the export end-point as /collections/{collectionName}/export.jsonl? It indicates that you are exporting the contents of the collection as jsonl

Yeah that works.
On 2nd thought, this could also mean that you're exporting the collection, meaning the schema that you posted, in addition to the documents. So it is a little confusing. Let me think about alternatives.

Although, I think we should probably use the Content-Type header to indicate the format instead of putting it in the URL. May be application/jsonl?

@jasonbosco
Copy link
Member Author

jasonbosco commented Jan 5, 2018

Btw, we have the same conceptual problem with the search endpoint, but the server already handles this case fine:

/collections/{collectionName}/documents/search collides with /collections/{collectionName}/documents/{id}

So we've actually already blacklisted search from being a valid document ID

@jasonbosco
Copy link
Member Author

Here's an idea to solve both the export and search endpoints:

GET /collections/{collectionName}/documents?q=Stark&filter_by=... for the search endpoint - you're essentially GETing documents and filtering when you do a search. We'll have to change the format of the returned results though in this case.

GET /collections/{collectionName}/documents with a Content-Type of application/jsonl will export all documents in JSONL format.

The nice thing about this is that you could also export the results of a search in JSONL format. So it's the same endpoint serving both purposes.

Let me know if this works and I can massage the Response data model appropriately in the spec.

@kishorenc
Copy link
Member

kishorenc commented Jan 5, 2018

Btw, we have the same conceptual problem with the search endpoint, but the server already handles this case

I just realized that we can handle this easily if we place the /documents/:id routes right at the bottom after /documents/search and /documents/export. That's why it worked for the search end-point. This is an easy change, so I recommend that we do that. We don't expect to have too many /documents/X endpoints so it should be fine. Even Twitter and Github have the same problem (with the username in the URL) and they are fine :)

If this is fine, I will go ahead and re-order the routes.

@jasonbosco
Copy link
Member Author

Yup, that works!

@kishorenc
Copy link
Member

Fixed - you can pull the docker image again and test.

@jasonbosco
Copy link
Member Author

jasonbosco commented Jan 5, 2018

Yup, works 👍

I've updated the API spec in typesense/typesense-api-spec#3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants