Figure out how to deal with leaky comment IDs in the web API #652

Closed
opened 2026-07-08 15:30:02 +00:00 by nphilipp · 5 comments
Member

Description

We are looking for a way to identify comments in the public web API.

Acceptance Criteria

  • One or more approaches to ensure unique "keys" for public and private comments are identified and documented. They must ensure that existing comments retain their ID values as keys.

Background

  • Currently, Comment.ID is leaked into the public API.
  • Private comments live in a separate table and have their own ID fields, so just this number alone isn’t unique to address a comment.
  • For the future it is planned that private issues (and their comments) can be converted to public ones (and vice versa), so one issue may have both comments that were originally private and public, i.e. we need something which identifies any comment, public or private, uniquely.
  • It is not necessary that what identifies a comment in the public API remains being mapped to the ID field of Comment and/or PrivateComment.
  • It is necessary that existing IDs are grandfathered into the new scheme, i.e. that for existing comments, their public IDs keep on working.
# Description We are looking for a way to identify comments in the public web API. # Acceptance Criteria - [x] One or more approaches to ensure unique "keys" for public and private comments are identified and documented. They must ensure that existing comments retain their `ID` values as keys. # Background - Currently, `Comment.ID` is leaked into the public API. - Private comments live in a separate table and have their own `ID` fields, so just this number alone isn’t unique to address a comment. - For the future it is planned that private issues (and their comments) can be converted to public ones (and vice versa), so one issue may have both comments that were originally private and public, i.e. we need something which identifies any comment, public or private, uniquely. - It is not necessary that what identifies a comment in the public API remains being mapped to the `ID` field of `Comment` and/or `PrivateComment`. - It is necessary that existing IDs are grandfathered into the new scheme, i.e. that for existing comments, their public IDs keep on working.
nphilipp added this to the Sprint 24 project 2026-07-08 15:30:02 +00:00
Author
Member

Thoughts:

  • We need …:
    • … either a common source of IDs
    • … or separate namespaces (say, add a prefix to private comments)
  • If we used prefixes for private comments, we need to store this somewhere, so it is retained when it’s converted to public (same vice versa)
  • Same if we have a common source for external IDs that isn’t used in the ID fields of both involved tables
  • Prefixes are ugly, and potentially misleading: e.g. if private comments had a prefix of "P", and the comment gets converted, someone dealing with it could think it is a private comment
  • If we wanted IDs to be shared between the tables, and the source of IDs to be external to both tables, we would have to remove the effects of autoincr from the ID primary key field of Comment. This could require recreating the table (depending on how we go about it), which is complicated.
Thoughts: - We need …: - … either a common source of IDs - … or separate namespaces (say, add a prefix to private comments) - If we used prefixes for private comments, we need to store this somewhere, so it is retained when it’s converted to public (same vice versa) - Same if we have a common source for external IDs that isn’t used in the ID fields of both involved tables - Prefixes are ugly, and potentially misleading: e.g. if private comments had a prefix of "P", and the comment gets converted, someone dealing with it could think it is a private comment - If we wanted IDs to be shared between the tables, and the source of IDs to be external to both tables, we would have to remove the effects of `autoincr` from the `ID` primary key field of `Comment`. This could require recreating the table (depending on how we go about it), which is complicated.
Author
Member

My preferred option would be to “steal” the auto-incrementing Comment.ID for creating PrivateComment entries:

  • Embed the original Comment type into PrivateComment and Override the ID field so it doesn’t get autoincr (just in case). Because embedding one type into another changes how literals of the extended type work (… in hundreds of places …), this is not really feasible. But we don’t really want to change the structure, just how XORM creates the table column, so we can use the reflect package to synthesize a modified copy of the type (which just changes the XORM tag on the ID column) and use that at startup for db.RegisterModel().
  • When creating a private comment, create an empty public comment first, “steal” the (unique) ID, delete the public comment and create the private comment with the “stolen” ID.

Upside: unique IDs make linking to them clearly “just happen” and seamless conversion between public and private easy. No need to store an original ID or similar.

Downside: It makes both creating a new private comment in the database and creating the table in the first place a little more involved.

**My preferred option** would be to “steal” the auto-incrementing `Comment.ID` for creating `PrivateComment` entries: - ~~Embed the original `Comment` type into `PrivateComment` and~~ Override the `ID` field so it doesn’t get `autoincr` (just in case). Because embedding one type into another changes how literals of the extended type work (… in hundreds of places …), this is not really feasible. But we don’t really want to change the structure, just how XORM creates the table column, so we can use the `reflect` package to synthesize a modified copy of the type (which just changes the XORM tag on the ID column) and use that at startup for `db.RegisterModel()`. - When creating a private comment, create an empty public comment first, “steal” the (unique) ID, delete the public comment and create the private comment with the “stolen” ID. Upside: unique IDs make linking to them clearly “just happen” and seamless conversion between public and private easy. No need to store an original ID or similar. Downside: It makes both creating a new private comment in the database and creating the table in the first place a little more involved.
Author
Member

An ostensibly cleaner variation would be to have a third table only for the IDs, but then you’d have to start looking them up elsewhere (whenever you need to look up an ID/a key).

An ostensibly cleaner variation would be to have a third table only for the IDs, but then you’d have to start looking them up elsewhere (whenever you need to look up an ID/a key).
Author
Member

For completeness sake, we could use prefixes:

Upside: we wouldn’t have to deal with ID fields in Comment vs. PrivateComment any special

Downside: we’d have to store this for the key to be stable across conversions public <=> private, and it would look funky to have a prefixed key for a public comment

For completeness sake, we could use prefixes: Upside: we wouldn’t have to deal with ID fields in Comment vs. PrivateComment any special Downside: we’d have to store this for the key to be stable across conversions public <=> private, and it would look funky to have a prefixed key for a public comment
Author
Member

A non-option would be to just not care, simply use the database IDs verbatim and cross fingers that we don’t run into collisions. Which would admittedly be rare, but this doesn’t mean impossible.

Another one would be switching to uuids, which would be a good idea if we started from scratch, but this would break existing links to comments.

A non-option would be to just not care, simply use the database IDs verbatim and cross fingers that we don’t run into collisions. Which would admittedly be rare, but this doesn’t mean impossible. Another one would be switching to uuids, which would be a good idea if we started from scratch, but this would break existing links to comments.
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
forge/forge#652
No description provided.