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

Add 'chck operation' endpoint #35

Closed
wants to merge 5 commits into from

Conversation

JAG-UK
Copy link
Contributor

@JAG-UK JAG-UK commented Sep 10, 2024

Fixes #23

Fixes #23

Signed-off-by: Jon Geater <[email protected]>
@SteveLasker
Copy link
Collaborator

Editors meeting suggestion: add a correlation id for the registration to coordinate on long-running operation requests.


The response contains a reference to the running operation which will eventually be available for the Signed Statement.

If 202 is returned, then clients should wait until Registration succeeded or failed by polling the Check Operation endpoint using the identifier returned in the response.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Concern for a failed registration returning not found, as the client will not know if it failed due to registration policy, or failed due to a network connection and continues to try.

Copy link
Collaborator

@OR13 OR13 left a comment

Choose a reason for hiding this comment

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

Can we show some error states for cases where the server rejects the operation id the client chose?

@SteveLasker
Copy link
Collaborator

Question from the editor's call: Was the request ID also meant to be the operationId, or was it intended to be a tuple where the client can submit a unique ID that needs to be validated? However, can the server return a guaranteed unique ID? The client submitting a unique ID is part of the idempotent design pattern.

Some background on idempotent patterns

Do we need a request ID (Idempotency-Key)?

  1. If the client makes a registration request and fails to get a response, the client is unsure if the registration was received, the response failed, or the request was never received.
  2. The client would try again, which could generate multiple registrations

In an idempotent world, the server would check if the request was previously made and, if so, return the same response.
The issue is that a client could be intentionally making a secondary registration because the registration policy may have changed or the client needs a new registration for a new point in time.

  • Do we need to support multiple registrations of the same signed statement from the same issuer?
    • Yes, as client may be registering, knowing a new registration policy has been set, or they simply need a new registered timestamp. If the client wishes to register the same signed statement multiple times, they would generate a unique Idempotency-Key
  • Can the server specify a window of time (which is implementation-specific) by which it treats them as idempotent?
  • How short should validity window be?
    • This is likely a new issue, with a SHOULD that would hopefully represent weeks or months, if not years.

Separately, a discussion to support success/failure response codes for failed registration vs. "not found" is needed.

@JAG-UK
Copy link
Contributor Author

JAG-UK commented Sep 18, 2024

Question from the editor's call: Was the request ID also meant to be the operationId, or was it intended to be a tuple where the client can submit a unique ID that needs to be validated? However, can the server return a guaranteed unique ID? The client submitting a unique ID is part of the idempotent design pattern.

The addition of the request_id in Commit 4 came out of the discussion in the prior editors' call where a couple of people asked for some kind of a correlation ID that the client could use to keep its own track of the LROs it was checking and receiving. Various use cases were mooted all along the lines that contexts may be confused (in the case of connection pooling or similar at the client end) or lost (in the case of one end or other crashing).

This seemed reasonable to try to accommodate and I did it to show how it might look, but actually I don't like how it's turned out and would gladly revert. Reasons:

  • Having the operation ID and request/correlation ID was redundant...unless we also enabled LIST on /operations which doesn't seem terribly sensible. That's why I removed the server-generated ID from the resource path.
  • Inserting the optional request ID into the POST required us to throw it in the unprotected header, which feels grotty

In an idempotent world, the server would check if the request was previously made and, if so, return the same response. The issue is that a client could be intentionally making a secondary registration because the registration policy may have changed or the client needs a new registration for a new point in time.

I like the idempotency key stuff and we should look into it but I think it's a reasonable and coherent v1 to just say that EVERY POST is a new registration and starts a new registration operation for a new receipt. We can't legislate for software bugs or try to guess the intention of the client.

Separately, a discussion to support success/failure response codes for failed registration vs. "not found" is needed.

Agreed. That's already on the list.

@ad-l
Copy link

ad-l commented Sep 19, 2024

I like the idempotency key stuff and we should look into it but I think it's a reasonable and coherent v1 to just say that EVERY POST is a new registration and starts a new registration operation for a new receipt. We can't legislate for software bugs or try to guess the intention of the client.

Yes, I think I am convinced by this argument, the request_id returned for the /operations call is a unique identifier for the registration and is enough for clients. The ability to set request_id in the registration request is unnecessary and may be confusing. I would also suggest to rename request_id to registration_id to make it clear that it is the identifier picked by the server when the registration is accepted

@ad-l
Copy link

ad-l commented Sep 19, 2024

@SteveLasker
Copy link
Collaborator

Meeting notes:
This PR dealt with several aspects, making it a bit harder to merge.
Jon will create separate PRs, based on work for 121 hackathon.

@SteveLasker SteveLasker deleted the jag-uk/23-distinct-async-operations branch December 31, 2024 20:58
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.1.2 Registration is Running
4 participants