Conversation
* [DAPS-1770] - release: v4.0.0 * [DAPS-1585] - update: dependencies, upgrade ssl dependency. 3.2.5 (#1646) * [DAPS-1605] - fix: scripts, install_foxx.sh by splitting ssl_args (#1623) * [DAPS-1651] - refactor: scripts, compose, univify treatment of env variables in compose env generator (#1656) (#1658) * [DAPS-1675] - feature: foxx, adding the logger functions for future PR's (#1675) * [DAPS-1659] - refactor: scripts, remove dependencies install scripts (#1660) * [DAPS-1670] - feature: common, core, repo, python_client, web, allow passing repo types in protobuf messages (#1670) * [DAPS-1671] - feature: foxx, add repository and execution strategy types (#1672) * [DAPS-1661] - refactor: compose, scripts, remove remaining occurences of zeromq system secret. (#1661) * [DAPS-1522] - refactor: foxx, user router logging improvements, remove non helpful logs from tasks.js (#1629) * [DAPS-1691] - refactor: foxx, adjust validation.js swap g_lib with error_code require rem… (#1691) * [DAPS-1692] - tests: ci, End-to-end web tests, fix flaky test (#1693) * [DAPS-1694] - refactor: foxx, move permissions functions from support.js to lib/permissions (#1695) * [DAPS-1685] - feature: compose, enable arangodb ssl (#1687) * [DAPS-1700] - fix: ci, limit arangodb job output to last 3 hours. (#1701) * [DAPS-1676] - feature: foxx, arango add factory for repositories for metadata and globus (#1697) * [DAPS-1718] - feature: web, core, python client, Protobuf ExecutionMethod enum, add RepoAllocationCreateResponse (#1719) * [DAPS-1713] - refactor: core, web, python client, protobuf, allow optional fields when creating repo to support metadat… (#1714) * [DAPS-1715] - refactor: core, make path, pub_key, address, endpoint optional in repoCreateRequest (#1716) * [DAPS-1705] - feature: foxx, integrate metadata globus factory repo router create (#1706) * [DAPS-1688] - update: dependencies, core, repo, authz, gcs, Crypto libssl switched to version 3 globus_sdk version pinned (#1689) * [DAPS-1729] - fix: ci, downstream datafed dependencies pipelines are building the container image from incorrect sha (#1732) * [DAPS-1711] - refactor: foxx standardize repo response schema (#1712) * [DAPS-1725] - refactor: remove confusing apache conf file. (#1728) * [DAPS-1707] - update: dependencies, web, update web dependencies before install (#1709) * [DAPS-1522] - refactor: foxx, task router logging improvements (#1648) * [DAPS-1522] - refactor: foxx, query router logging improvements (#1627) * [DAPS-1735] - bug: foxx, remove duplicate user_router test (#1736) * [DAPS-1731] - reature: scripts, compose, add scripts to generate globus credentials for web service (#1731) * [DAPS-1725] - refactor: tests, mock core server centralized (#1726) * [DAPS-1741] - update: scripts, native client id in intialize_globus_endpoint and globus_clea… (#1741) * [DAPS-1745] - fix: scripts, account for nested client credentials. (#1746) * [DAPS-1725-2] - fix; tests, centralized mock core service libraries fixed (part 2) (#1747) * [DAPS-1742] - refactor script replace os.path.join with urllib.parse.urljoin (#1744) * [DAPS-1749] - refactor: cmake, set cmake policy to silence noisy warning. (#1750) * [DAPS-1522] - refactor: foxx, feature tag router logging improvements (#1734) * [DAPS-1378] - fix: web, mapping of multiple globus accounts. (#1753) * [DAPS-1756] - fix: scripts, foxx, add retries and connection check to install_foxx.sh script. (#1757) * [DAPS-1522] - refactor: foxx, Version Router Logging Improvements (#1758) * [DAPS-1737] - refactor: compose, cleanup arango ssl env variables (#1765) * [DAPS-1766] - fix: ci, python client provisioning job * [DAPS-1663] - feature: core Service, adding Correlation ID to Logging (#1704) Co-authored-by: Aaron Perez <perezam@ornl.gov> Co-authored-by: AronPerez <aperez0295@gmail.com> Co-authored-by: Blake Nedved <nedvedba@ornl.gov> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Co-authored-by: nedvedba <145805866+nedvedba@users.noreply.github.com> Co-authored-by: Austin Hampton <amh107@latech.edu> Co-authored-by: Austin Hampton <44103380+megatnt1122@users.noreply.github.com> Co-authored-by: Blake Nedved <blakeanedved@gmail.com> Co-authored-by: Polina Shpilker <infinite.loopholes@gmail.com> * [DAPS-1777] - Foxx, fix: user_router fix regression in missing response. (#1778) [DAPS-1786] - Web tests, refactor: add test for hitting password reset. (#1787) * [DAPS-1774] - Core, Python, Database, Foxx, Test add query end to end tests (#1779) * [DAPS-1775] - fix: core, foxx, add missing {}, foxx query_router add params object schema to routes. (#1781) * [DAPS-1777] - fix: foxx, user_router fix regression in missing response. (#1778) * [DAPS-1786] - refactor: web tests, add test for hitting password reset. (#1787) * [DAPS-1277] - fix: mock, core, common, PROXY_BASIC_ZMQ and PROXY_CUSTOM correctly defined * [DAPS-1790] - fix: common, core, repo, zmq assertion failure during EXCEPT call due to callin zmq_msg with invalid state after closing it. * [DAPS-1791] - fix: build, python client, requirements.txt was being moved to a folder named requirements.txt during cmake configure script. * chore: Auto-format JavaScript files with Prettier * [DAPS-1774] - continuation of fix (#1798) * [DAPS-1774] - continuation of fix (#1798) * release: version 4.0.1 (#1807) * refactor: only print subset of user properties. (#1804) * refactor: only print subset of user properties. * chore: Auto-format JavaScript files with Prettier * [DAPS-1806] - refactor: foxx, user tokens expiring route. (#1806) --------- Co-authored-by: Aaron Perez <perezam@ornl.gov> Co-authored-by: AronPerez <aperez0295@gmail.com> Co-authored-by: Blake Nedved <nedvedba@ornl.gov> Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com> Co-authored-by: nedvedba <145805866+nedvedba@users.noreply.github.com> Co-authored-by: Austin Hampton <amh107@latech.edu> Co-authored-by: Austin Hampton <44103380+megatnt1122@users.noreply.github.com> Co-authored-by: Blake Nedved <blakeanedved@gmail.com> Co-authored-by: Polina Shpilker <infinite.loopholes@gmail.com>
Reviewer's GuideThis PR updates several Foxx router handlers in user_router.js and query_router.js to standardize how the client user object is retrieved and logged, and to enrich request logging with additional contextual information for user update and identity operations. Sequence diagram for user update route loggingsequenceDiagram
actor Client
participant UserRouter
participant g_lib
participant logger
participant Database
Client->>UserRouter: HTTP GET /user/update?client&subject
UserRouter->>g_lib: getUserFromClientID(client)
g_lib-->>UserRouter: client_object
UserRouter->>logger: logRequestStarted(client._id, correlationId, GET, basePath + /update, Started, Update user information)
alt subject query param present
UserRouter->>Database: lookup user by subject
Database-->>UserRouter: user_record
else no subject param
UserRouter->>Database: lookup user by other criteria
Database-->>UserRouter: user_record
end
UserRouter->>Database: update user_record
Database-->>UserRouter: updated_user
UserRouter->>UserRouter: build result = [updated_user]
UserRouter->>UserRouter: extract is_admin, max_coll, max_proj, max_sav_qry
UserRouter->>UserRouter: build extra_log_info with quotas and admin flag
UserRouter-->>Client: HTTP 200 [updated_user]
Sequence diagram for identity add route loggingsequenceDiagram
actor Client
participant UserRouter
participant g_lib
participant logger
participant Database
Client->>UserRouter: HTTP GET /user/ident/add?client&ident
UserRouter->>g_lib: getUserFromClientID(client)
g_lib-->>UserRouter: client_object
UserRouter->>logger: logRequestStarted(client._id, correlationId, GET, basePath + /ident/add, Started, Add new linked identity)
UserRouter->>g_lib: isUUID(ident)
g_lib-->>UserRouter: boolean
alt ident is valid UUID and not already linked
UserRouter->>Database: create linked identity
Database-->>UserRouter: new_identity
else invalid or duplicate ident
UserRouter->>UserRouter: handle error or no-op
end
UserRouter-->>Client: HTTP response (success or error)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the routes where
const client = ...was changed toclient = ...(e.g., user update and identity-add), ensureclientis explicitly declared in the appropriate scope (e.g.,let client = null;at the top of the handler) to avoid creating or mutating an implicit/global variable across requests. - The new
extra_log_infoobject in the user update handler is assigned without a local declaration; declare it (e.g.,let extra_log_info = {...}) in the correct scope to avoid implicit globals and to make its lifecycle explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the routes where `const client = ...` was changed to `client = ...` (e.g., user update and identity-add), ensure `client` is explicitly declared in the appropriate scope (e.g., `let client = null;` at the top of the handler) to avoid creating or mutating an implicit/global variable across requests.
- The new `extra_log_info` object in the user update handler is assigned without a local declaration; declare it (e.g., `let extra_log_info = {...}`) in the correct scope to avoid implicit globals and to make its lifecycle explicit.
## Individual Comments
### Comment 1
<location path="core/database/foxx/api/user_router.js" line_range="406" />
<code_context>
+
+ const { is_admin, max_coll, max_proj, max_sav_qry } = user.new;
+
+ extra_log_info = {
+ is_admin,
+ max_coll,
</code_context>
<issue_to_address>
**issue (bug_risk):** `extra_log_info` is assigned without declaration, which can create or mutate a global variable.
Assigning to `extra_log_info` without `const`/`let`/`var` will either create/mutate a global (non‑strict) or throw (strict), and obscures its scope. If this should be local, declare it; if it’s part of an outer logging context, reference that explicitly instead of relying on an implicit global.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| const { is_admin, max_coll, max_proj, max_sav_qry } = user.new; | ||
|
|
||
| extra_log_info = { |
There was a problem hiding this comment.
issue (bug_risk): extra_log_info is assigned without declaration, which can create or mutate a global variable.
Assigning to extra_log_info without const/let/var will either create/mutate a global (non‑strict) or throw (strict), and obscures its scope. If this should be local, declare it; if it’s part of an outer logging context, reference that explicitly instead of relying on an implicit global.
Ticket
Description
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Improve user and query API logging around client-derived operations and align client variable handling in affected routes.
Bug Fixes:
Enhancements: