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

Improve API Methods #129

Closed
RyanThompson opened this issue Dec 16, 2022 · 13 comments
Closed

Improve API Methods #129

RyanThompson opened this issue Dec 16, 2022 · 13 comments

Comments

@RyanThompson
Copy link
Contributor

GET /api/streams/{stream}/entries

  • Perhaps rudimentary filtering only using query string? name=foo&bar=baz
  • If you need other constraint operators use POST /api/streams/{stream}/entries/new-query
  • Might be worth dropping?

POST /api/streams/{stream}/entries/{criteria-method}

  • Payload of JSON query stack is supported
  • Criteria always (less get/first) returns the query criteria

GET /api/streams/{stream}/entries/{repository-method}

  • Supports query string parameters to pass to the repository method
  • Repositories return instances or collections of them only - no need for JSON criteria payload.

This will allow easier extension using logic that belongs in these patterns in the first place, removing the need for API specific controllers to be used just to access methodology in the design patterns mentioned above. Annotate and use. Use them all the same in your codebase.

Safelist and perhaps use annotations to denote which methods are API safe.

@RyanThompson
Copy link
Contributor Author

We have two methods that I think are too similar to keep - but I want to see what anyone here might think:

GetEntries = GET /api/streams/{stream}/entries
QueryEntries = POST /api/streams/{stream}/query

GetEntries is GET only and supports a LIMITED fixed filtering style using the query string.
QueryEntries is POST and supports the full query/criteria builder API through JSON payload. Including custom methods.

Both methods currently force pagination (even if you ask for 999,999 per page).

https://github.com/laravel-streams/streams-api/blob/1.0/tests/Http/Controller/Entries/GetEntriesTest.php
https://github.com/laravel-streams/streams-api/blob/1.0/tests/Http/Controller/Entries/QueryEntriesTest.php

A repository endpoint also allows calling repository methods and passing query string payloads—a tentative method right now; also not sure if it's necessary. I don't like that repositories can return collections or single entries. So that would mean the API endpoint may do the same depending on the return of the repository method...

@RyanThompson
Copy link
Contributor Author

Removed the repository method.

@RyanThompson
Copy link
Contributor Author

Ugh, we need GET on querying because of cache control.

@RobinRadic
Copy link

RobinRadic commented Dec 28, 2022

I don't really like the idea of 3 different endpoints for pretty much the same data.

Query strings have quite some limitations in terms of nested key-value stuff.
GET /api/streams/{stream}/entries
Should only accept a JSON request body. (Actually, i'd say make that so for every endpoint. Consistent and flexible)

By using a JSON request body like:

{
    where   : [
        [ 'a', '==', 'b' ],
        [ 'b', '==', 'where it\'s always super & nice' ],
    ],
    per_page: 10,
    paginate: true,
}

We can easily pass that to the Criteria instance in the backend. We can combine multiple criteria methods, including pagination and be awesome with 1 single endpoint.

@RyanThompson
Copy link
Contributor Author

It seems as though GET and request bodies violate standards: https://stackoverflow.com/questions/978061/http-get-with-request-body

@RyanThompson
Copy link
Contributor Author

I am thinking that we need a query method using post to query with request body. Won’t be technically idempotent, which is accurate.. and we can cache differently, internally.

Agreed on the duplicate endpoints though.

@RyanThompson
Copy link
Contributor Author

Ah, using “Expires header or a Cache-Control.”

So question is, do we need a repository GET method. And the POST method will utilize request body and support criteria methods.

Thinking about stuff like get home page, get navigation, stuff that’s potentially constantly used.

@RobinRadic
Copy link

RobinRadic commented Jan 1, 2023

It seems as though GET and request bodies violate standards

It does not actually violate it i think. From the same stackoverflow answer:

The RFC2616 referenced as "HTTP/1.1 spec" is now obsolete. In 2014 it was replaced by RFCs 7230-7237. Quote "the message-body SHOULD be ignored when handling the request" has been deleted. It's now just "Request message framing is independent of method semantics, even if the method doesn't define any use for a message body" The 2nd quote "The GET method means retrieve whatever information ... is identified by the Request-URI" was deleted. - From a comment

From the HTTP 1.1 2014 Spec:

A payload within a GET request message has no defined semantics; sending a payload body on a GET request might cause some existing implementations to reject the request.

Also interesting is:

Elasticsearch is a fairly major product that utilises HTTP request bodies in GET

So i still believe having a GET request body is worth considering.

IMO POST should be used to create a resource, not to query/fetch resource.

POST to a URL creates a child resource at a server defined URL.
The relevant specification for POST is RFC 2616 §9.5ff.

@RyanThompson
Copy link
Contributor Author

The whole thing with JS flat-out not supporting it is a hard blocker.

If it helps, we can look at it as creating a new query. And we still have caching opportunities.

@RyanThompson
Copy link
Contributor Author

GET /streams/{stream}/entries/{method?} -> Repository::$method(...[$query])
POST /streams/{stream}/entries/{method?} -> Criteria::$method(...[$query])...applyQuery(json_decode($body))->get()/->first()

@RyanThompson
Copy link
Contributor Author

Gonna tighten and write that - update tests. Pretty close to that now already.

This is going to require tapping into repository reading via attributes at some point to document the non-universal endpoints. Repository returns, input/parameters, etc, all need generated from the attributes.

@RyanThompson
Copy link
Contributor Author

Going to move this repository/criteria idea to a separate issue and label it for post-release. Current routes work well and are all tested. @RobinRadic would be an ideal time to give into the ApiClient

Quick and dirty README and checkout tests while I work on docs: https://github.com/laravel-streams/streams-api

@RyanThompson
Copy link
Contributor Author

#139

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

No branches or pull requests

2 participants