permission: reuse cached env strings - #64912
Conversation
Permission::is_scope_granted() and Permission::Drop() built the "permission" and "resource" diagnostics message keys with FIXED_ONE_BYTE_STRING(isolate, ...) on every publish, even though Environment already caches these exact strings via env->permission_string() and env->resource_string() (src/env_properties.h). One call site in the same file already used the cached accessor; these two did not, and Environment is already in scope at both sites. Assisted-by: Claude Sonnet 5 Signed-off-by: agape1225 <49804691+agape1225@users.noreply.github.com>
|
Review requested:
|
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #64912 +/- ##
==========================================
- Coverage 90.29% 90.27% -0.03%
==========================================
Files 760 760
Lines 247061 247061
Branches 46585 46587 +2
==========================================
- Hits 223092 223035 -57
- Misses 15451 15496 +45
- Partials 8518 8530 +12
π New features to boost your workflow:
|
Follow-up to nodejs#59891, covering files it missed. Overlapping locations (node_ffi.cc, crypto_util.cc) were already handled by nodejs#64760. Refs: nodejs#59891 Refs: nodejs#64760 Refs: nodejs#64912 Signed-off-by: leah-1ee <selee3196@gmail.com>
Follow-up to nodejs#59891, covering files it missed. Overlapping locations (node_ffi.cc, crypto_util.cc) were already handled by nodejs#64760. Refs: nodejs#59891 Refs: nodejs#64760 Refs: nodejs#64912 Signed-off-by: leah-1ee <selee3196@gmail.com>
Follow-up to #59891, covering files it missed. Overlapping locations (node_ffi.cc, crypto_util.cc) were already handled by #64760. Refs: #59891 Refs: #64760 Refs: #64912 Signed-off-by: leah-1ee <selee3196@gmail.com> PR-URL: #65039 Refs: #59891 Refs: #64760 Refs: #64912 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Commit Queue failed- Loading data for nodejs/node/pull/64912 β Done loading data for nodejs/node/pull/64912 ----------------------------------- PR info ------------------------------------ Title permission: reuse cached env strings (#64912) β Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch agape1225:permission-use-cached-strings -> nodejs:main Labels c++, author ready, needs-ci, commit-queue Commits 1 - permission: reuse cached env strings Committers 1 - agape1225 <49804691+agape1225@users.noreply.github.com> PR-URL: https://github.com/nodejs/node/pull/64912 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/64912 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> -------------------------------------------------------------------------------- βΉ This PR was created on Sat, 01 Aug 2026 13:16:38 GMT β Approvals: 2 β - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/64912#pullrequestreview-4838784169 β - Daeyeon Jeong (@daeyeon): https://github.com/nodejs/node/pull/64912#pullrequestreview-4876387866 β Last GitHub CI successful βΉ Last Full PR CI on 2026-08-09T11:58:23Z: https://ci.nodejs.org/job/node-test-pull-request/75689/ - Querying data for job/node-test-pull-request/75689/ β Build data downloaded β Last Jenkins CI successful -------------------------------------------------------------------------------- β No git cherry-pick in progress β No git am in progress β No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/main up to date... From https://github.com/nodejs/node * branch main -> FETCH_HEAD ad3432189c..4a4cc1ed24 main -> origin/main β origin/main is now up-to-date main is out of sync with origin/main. Mismatched commits: - ff1744022d build: deprecate always enabled `--enable-static` - 4a4cc1ed24 build: deprecate always enabled `--enable-static` -------------------------------------------------------------------------------- HEAD is now at 4a4cc1ed24 build: deprecate always enabled `--enable-static` β Reset to origin/main - Downloading patch for 64912 From https://github.com/nodejs/node * branch refs/pull/64912/merge -> FETCH_HEAD β Fetched commits as 5f1ef0a95376..615612e04c54 -------------------------------------------------------------------------------- Auto-merging src/permission/permission.cc The previous cherry-pick is now empty, possibly due to conflict resolution. If you wish to commit it anyway, use:https://github.com/nodejs/node/actions/runs/31401397043 |
|
Superseded by 41afbd3 |
This PR replaces
FIXED_ONE_BYTE_STRING(isolate, "...")literals insrc/permission/permission.ccwith the Environment-cachedpermission_string()/resource_string()accessors already usedelsewhere in the same file. Follow-up to #59891, which didn't cover
this file.