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

[Sever v3] Split CertificateBasedActionIdempotentOutcome into specific outcomes #7279

Open
vxgmichel opened this issue May 7, 2024 · 1 comment
Labels
A-Server Area: Server application I-Postgresql Impact: Postgresql issue refactor Improve code, don't change behavior
Milestone

Comments

@vxgmichel
Copy link
Contributor

At the moment, the CertificateBasedActionIdempotentOutcome internal outcome is used as placeholder for different outcomes containing a timestamp and leading to a reply of the form Rep*Already*:

@dataclass(slots=True)
class CertificateBasedActionIdempotentOutcome(BadOutcome):
certificate_timestamp: DateTime

In paticular, it's used for the following replies:

  • authenticated_cmds.latest.realm_create.RepRealmAlreadyExists
  • authenticated_cmds.latest.realm_share.RepRoleAlreadyGranted
  • authenticated_cmds.latest.realm_unshare.RepRecipientAlreadyUnshared
  • authenticated_cmds.latest.realm_rename.RepInitialNameAlreadyExists
  • authenticated_cmds.latest.user_revoke.RepUserAlreadyRevoked

This is confusing, especially since the reply itself is properly specialized. Instead, we should use 5 properly named outcome for those 5 replies.

Note that other outcomes with a timestamp exists, such as:

  • BadKeyIndex (which should be renamed BadKeyIndexOutcome for consistency)
  • RepSequesterInconsistency (which should be renamed RepSequesterInconsistencyOutcome for consistency)1

Also note that some other replies including the name Already do not provide a timestamp, for instance:

  • OrganizationBootstrapStoreBadOutcome.ORGANIZATION_ALREADY_BOOTSTRAPPED
  • OrganizationCreateBadOutcome.ORGANIZATION_ALREADY_EXISTS
  • BlockCreateBadOutcome.BLOCK_ALREADY_EXISTS
  • InviteNewForUserBadOutcome.CLAIMER_EMAIL_ALREADY_ENROLLED
  • InviteCancelBadOutcome.INVITATION_ALREADY_DELETED
  • UserCreateUserStoreBadOutcome.USER_ALREADY_EXISTS
  • UserCreateDeviceStoreBadOutcome.DEVICE_ALREADY_EXISTS
  • VlobCreateBadOutcome.VLOB_ALREADY_EXISTS

This raises the question of why some replies/outcome include a timestamp and some don't. This choice seems to depend on certain clients use cases, which indicates a strong coupling between the client and the server. Instead, my opinion is that all errors based on existing information should include the timestamp of when that information became valid in the system. Note that some errors do not depend on a specific existing information, such as all the X_NOT_FOUND errors.

Footnotes

  1. Sidenote: SequesterInconsistency, SequesterServiceNotAvailable and RejectedBySequesterService do not inherit from BadOutcome, which should be fixed when sequester gets properly implemented.

@vxgmichel vxgmichel added I-Postgresql Impact: Postgresql issue A-Server Area: Server application refactor Improve code, don't change behavior labels May 7, 2024
@mmmarcos
Copy link
Contributor

This is not a priority but it seems be rather easy to implement.

@mmmarcos mmmarcos added this to the v3.1 milestone May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Server Area: Server application I-Postgresql Impact: Postgresql issue refactor Improve code, don't change behavior
Projects
None yet
Development

No branches or pull requests

2 participants