Skip to content

feat: replace ID with UID - #363

Open
sabhas wants to merge 9 commits into
mainfrom
issue-361
Open

feat: replace ID with UID#363
sabhas wants to merge 9 commits into
mainfrom
issue-361

Conversation

@sabhas

@sabhas sabhas commented May 9, 2023

Copy link
Copy Markdown
Member

BREAKING CHANGE: remove auto incremental ids from user, group and permissions and add a virtual uid property that returns string value of documents object id

Issue

Closes #361.

While addressing #359, the auto-incrementing sequence counter used for
User, Group, and Permission IDs was found to be broken on Cosmos DB
(it set every ID to 1). More generally, sequencing via a shared counter
document is not a good fit: it's a bottleneck on every
insert and a source of race conditions under concurrent writes.

Intent

Replace auto-incremental numeric IDs with a string uid across User,
Group, and Permission, and make the API consistently expose uid
instead of id wherever these entities appear in a response.

Implementation

  • Added a virtual uid property to the User, Group, and Permission
    Mongoose models, returning this._id.toString(). No new field is stored
    and no data migration is required — every document already has _id,
    old and new alike.
  • Removed the old sequencing mechanism entirely: the Counter model and
    getSequenceNextValue utility are gone, along with the bottleneck/race
    condition they caused.
  • Updated all API responses that expose a user, group, or permission
    identifier — including login (POST /SASLogon/login), session
    (GET /SASjsApi/session), and the user/group/permission endpoints
    — to consistently return uid.
  • Updated the web frontend (permission management, user profile, session
    restoration on page load) to read uid instead of id.

This is a breaking change for any existing client relying on numeric
IDs from this API — endpoints now return an opaque string identifier
instead.

Checks

  • Code is formatted correctly (npm run lint:fix).
  • Any new functionality has been unit tested.
  • All unit tests are passing (npm test).
  • All CI checks are green.
  • Reviewer is assigned.

sabhas added 4 commits May 9, 2023 15:01
BREAKING CHANGE: remove auto incremental ids from user, group and permissions and add a virtual uid property that returns string value of documents object id
@sabhas
sabhas requested a review from YuryShkoda May 11, 2023 06:21
Comment thread api/src/routes/api/spec/auth.spec.ts Outdated
Comment thread api/src/routes/api/spec/web.spec.ts Outdated
Comment thread web/src/context/appContext.tsx Outdated
Comment thread web/src/context/appContext.tsx Outdated
Comment thread web/src/utils/types.ts Outdated
Comment thread web/src/utils/types.ts Outdated
Comment thread web/src/utils/types.ts Outdated

@YuryShkoda YuryShkoda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see comments above

@sabhas
sabhas requested a review from YuryShkoda August 8, 2023 10:08
Execution.spec.ts, processProgram.spec.ts and code.spec.ts didn't
exist when issue-361 (ID -> UID) branched, so they were written
against the old userId: number shape. Update them to match the
merged-in string-based uid now that main has been merged in.
POST /SASLogon/login and GET /SASjsApi/session still returned the
old `id` field, while the rest of the ID->UID migration (#363)
standardized on `uid`. Not functionally broken - Mongoose provides
a built-in `id` virtual by default (_id.toHexString()) that happened
to resolve to the same value as the new `uid` virtual - but it's an
inconsistent public API surface, and relying on that coincidence
wasn't the intent of the migration.

Neither of these files was touched by any of issue-361's own
commits, so this predates the merge rather than being caused by it.

- web.ts: login response and session storage now source from
  user.uid explicitly
- session.ts: SessionResponse dropped its Omit<UserResponse, 'uid'>
  + id override in favor of just extending UserResponse
- verifyTokenInDB.ts: token-refresh path, same fix
- login.tsx / appContext.tsx: updated to read the corrected field

Verified with a real end-to-end request (genuine app boot, real
MongoDB, real CSRF handshake) - not just type-checking - to confirm
the actual HTTP response bodies carry uid, not id.
@YuryShkoda YuryShkoda self-assigned this Jul 15, 2026

@4gl-reviewer 4gl-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hermes Agent Code Review

Verdict: Request Changes

This PR replaces numeric auto-increment IDs with MongoDB _id-based UIDs (24-char hex strings) across the entire server codebase. The core approach is sound, but there are several critical issues that need to be addressed before merging.

Critical

  1. permission.ts line 341 — select: 'groupId name description' not updated to 'uid name description': In updatePermission, the group populate select still says groupId while every other select in the PR was changed to uid. The response will have a missing/null uid for the group object on PATCH permission. Should be .populate({ path: 'group', select: 'uid name description' }).

  2. desktop.ts line 6 — regex /^\/SASjsApi\/user\/[0-9]*$/ only matches numeric IDs: UIDs are now 24-char hex strings. In desktop mode, GET/PATCH to /SASjsApi/user/{uid} will be blocked by desktopRestrict, breaking desktop user profile access entirely. This line wasn't modified in the PR but is now broken by the ID→UID change. Update to /^\/SASjsApi\/user\/[0-9a-fA-F]{24}$/.

  3. seedDB.tsALL_USERS_GROUP name changed from 'AllUsers' to 'all-users' without migration: Existing deployments will get a duplicate group — new users join 'all-users' while existing users remain in 'AllUsers'. Permissions referencing the old group name are orphaned. The comment on line 27 still says 'AllUsers'. Either add a migration to rename the existing group, or keep the original name.

  4. Merge regression reverting PR #388: The issue-361 branch was based on a commit before the #388 fix (return 200 with log on SAS session failure). The merge into main reverted the fix — processProgram.ts now throws on SessionState.failed instead of resolving, Execution.ts wraps it in SessionExecutionError producing a 400, and tests were reverted to expect the old throwing behavior.

Warnings

  1. swagger.yaml SessionResponse still uses id not uid — Code returns uid but committed swagger shows id. Same for /SASLogon/login user object.
  2. Inconsistent param naming in UserControllergetUser uses @Path() uid but updateUser/deleteUser use @Path() userId with stale @example userId 1234.
  3. Missing uidValidation on POST /:groupUid/:userUid and DELETE /:groupUid/:userUid routes in group.ts.
  4. group.ts @example userId "12ByteString" should be @example userUid for the addUserToGroup method.

Looks Good

  • Core approach of using MongoDB _id as uid virtual is sound
  • Clean removal of Counter model and getSequenceNextValue
  • Good uidValidation with Joi (24-char hex) security addition
  • Frontend changes are consistent and complete
  • Test files properly updated throughout

Reviewed by Hermes Agent (GitHub App)

.populate({
path: 'group',
select: 'groupId name description -_id'
select: 'groupId name description'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: The group populate select here still says 'groupId name description', but every other select in this PR was changed to 'uid name description'. On a PATCH /SASjsApi/permission response, the group object will have a missing/null uid field. Should be:

.populate({ path: 'group', select: 'uid name description' })

Reviewed by Hermes Agent (GitHub App)

Comment thread api/src/utils/seedDB.ts

export const ALL_USERS_GROUP = {
name: 'AllUsers',
name: 'all-users',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: The ALL_USERS_GROUP name was changed from 'AllUsers' to 'all-users' without a migration. Existing deployments will get a duplicate group — new users join 'all-users' while existing users remain in 'AllUsers'. Permissions referencing the old group name are orphaned.

Also, the comment on line 27 still says 'AllUsers'.

Either add a migration to rename the existing group, or keep the original name.

Reviewed by Hermes Agent (GitHub App)

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.

replace ID with UID

2 participants