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

Cleanup references to agreed changes in API #23

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

caguado
Copy link
Collaborator

@caguado caguado commented Mar 8, 2024

This commit cleans up the references to the item_id and clarifies the HTTP return code for partial results.

Copy link
Collaborator

@jramseyer jramseyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few questions

* Contents: Session Array (all sessions in request accepted for configuration).
* 300:
* Contents: Modified Session Array, with rejected or additional sessions.
* Contents: Session Array (all sessions in request accepted for configuration). Should not all the sessions be accepted, the response also contains a list of sessions and the respective errors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(not blocking the merge) Is this a separate list, or will the rejected sessions be listed with status: "rejected"?
The second case makes sense to me, just confirming we are in alignment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response contains 2 lists, the list of sessions and a list of errors. Either can be full-empty, partial-partial, empty-full to signify what happened to the collection of requested resources. The sum of both should match that of the request.

The error code was under question and I haven't found a conclusive source on what 2xx matches a partial accepted request (206 implies content ranges and 207 caldav) so opted to not overload existing 2xx with additional semantics. In the same way we expect initiators to match by some property each item of the response to each item of the request, the absence of one successful can be matched against the errors in the error list.

@@ -359,11 +356,11 @@ On each call, there should be rate limits, allowed senders, and other optional r
* asn (requesting client's asn)
* request_id (optional, UUID of request)
* max_results (integer to indicate an upper bound for a given response page)
* next_token (opaque string to hint the query and last result returned when fetching a new page)
* next_token (opaque and optional string to hint the query and last result returned when fetching a previous page)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't next_token indicate the next page that we should fetch?
It comes from the previous page, but it links the next page, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does, the attribute itself is described as a token to convey that is abstract and opaque hence the explicit suffix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But doesn't the next token hint you to the next page, not the previous page?
Or is the intent here "give me the next page as directed from the 'next token' from the previous page"?

Maybe "opaque and optional string to hint the query and last result returned when fetching the following page" would be clearer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, let me be more verbose in the explanation to clarify it.

* Response:
* 200: OK
* Contents: Session Array of sessions in request_id, if provided. Else, all existing and in-progress sessions between client ASN and server.
* next_token (opaque string for clients to use when retrieving a new page)
* next_token (opaque string for clients to use when retrieving a new page and which missing indicates no more pages to retrieve)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: which -> when

@caguado caguado force-pushed the contrib/crlsa/fixup-item-errors branch 2 times, most recently from c4279bd to 413c179 Compare March 12, 2024 14:32
This commit cleans up the references to the item_id and clarifies the
HTTP return code for partial results.
@caguado caguado force-pushed the contrib/crlsa/fixup-item-errors branch from 413c179 to 5d95efc Compare March 12, 2024 14:35
Copy link
Collaborator

@jramseyer jramseyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you!

@jramseyer jramseyer merged commit f43f107 into main Mar 13, 2024
2 checks passed
@caguado caguado deleted the contrib/crlsa/fixup-item-errors branch March 13, 2024 18:32
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.

2 participants