Conversation
… 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.
* refactor: only print subset of user properties. * chore: Auto-format JavaScript files with Prettier
Contributor
Reviewer's GuideAdds end-to-end query API coverage and Playwright documentation, refines query payload handling and logging in Foxx services, switches servers to use a custom proxy implementation, bumps DataFed major versions to 2.x/4.x, and wires the new tests into CMake/CTest. Sequence diagram for query API execution with structured paramssequenceDiagram
actor Client
participant CoreServer as CoreServer_DatabaseAPI
participant Foxx as Foxx_QueryRouter
participant DB as ArangoDB
Client->>CoreServer: SearchRequest (params as JSON object)
CoreServer->>CoreServer: parseSearchRequest
CoreServer->>CoreServer: a_params = "{" + a_params + "}"
CoreServer->>CoreServer: generalSearch
CoreServer->>Foxx: HTTP POST /qry/exec
Note over CoreServer,Foxx: payload.params is serialized JSON object (no wrapping string)
Foxx->>Foxx: joi validation (params as object)
Foxx->>Foxx: execQuery(client, mode, published, query)
Foxx->>DB: AQL query with query.params
DB-->>Foxx: query results
Foxx-->>CoreServer: HTTP response (results)
CoreServer-->>Client: SearchReply
Sequence diagram for expiring token retrieval and loggingsequenceDiagram
actor Client
participant UserRouter
participant DB as ArangoDB
participant Logger
Client->>UserRouter: GET /token/get/expiring?expires_in
UserRouter->>Logger: logRequestStarted(desc)
UserRouter->>DB: AQL query for tokens expiring before now + expires_in
DB-->>UserRouter: results cursor
UserRouter->>UserRouter: extra_log_info = results.toArray()
UserRouter-->>Client: send(results)
UserRouter->>Logger: logRequestSuccess(desc, extra expiring_token_count)
Note over UserRouter,Logger: On failure, logRequestFailure(desc, expiring_token_count)
Updated class diagram for proxy factory and server usageclassDiagram
class IServer {
<<interface>>
+run()
}
class Proxy {
+Proxy(socket_options, socket_credentials, log_context)
+run()
}
class ProxyBasicZMQ {
+ProxyBasicZMQ(socket_options, socket_credentials, log_context)
+run()
}
class ServerFactory {
-LogContext m_log_context
+ServerFactory(log_context)
+create(type, socket_options, socket_credentials) IServer
}
class Server {
+msgRouter(log_context, thread_count)
+ioInsecure(log_context, thread_count)
}
ServerFactory ..> IServer : create
Proxy ..|> IServer
ProxyBasicZMQ --|> Proxy
ServerFactory ..> ProxyBasicZMQ : PROXY_BASIC_ZMQ
Server ..> IServer : uses proxy (PROXY_CUSTOM)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Playwright README Docker run example uses
-e DISPLAY=host.docker. internal:0, which has an extra space and stray dot; this should be corrected to something likehost.docker.internal:0so users can copy/paste it successfully. - In
tests/end-to-end/web-UI/CMakeLists.txtthere is a hardcodedmessage("ENABLE END TO END!!!!!!!")that will print on every configure; consider removing or guarding it to avoid noisy build output. - The query
paramshandling now expects an object in the Foxx routes (e.g.,query_router.jsschemas) whileDatabaseAPI::parseSearchRequeststill wrapsa_paramswith braces ("{" + a_params + "}"); aligning the representation used byDatabaseAPIand the Foxx API will prevent subtle mismatches in query parameter formatting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Playwright README Docker run example uses `-e DISPLAY=host.docker. internal:0`, which has an extra space and stray dot; this should be corrected to something like `host.docker.internal:0` so users can copy/paste it successfully.
- In `tests/end-to-end/web-UI/CMakeLists.txt` there is a hardcoded `message("ENABLE END TO END!!!!!!!")` that will print on every configure; consider removing or guarding it to avoid noisy build output.
- The query `params` handling now expects an object in the Foxx routes (e.g., `query_router.js` schemas) while `DatabaseAPI::parseSearchRequest` still wraps `a_params` with braces (`"{" + a_params + "}"`); aligning the representation used by `DatabaseAPI` and the Foxx API will prevent subtle mismatches in query parameter formatting.
## Individual Comments
### Comment 1
<location path="core/server/DatabaseAPI.cpp" line_range="1312" />
<code_context>
payload["qry_end"] = qry_end;
payload["qry_filter"] = qry_filter;
- payload["params"] = "{" + params + "}";
+ payload["params"] = params;
payload["limit"] = to_string(cnt);
</code_context>
<issue_to_address>
**issue (bug_risk):** The `params` field is still sent as a string while the Foxx API now expects a JSON object, which can break validation and query execution.
On the Foxx side, `params` is now validated as `joi.object()` and is no longer `JSON.parse`d from a string, but here `payload["params"]` is still assigned from a `std::string`. That will serialize as a JSON string value (e.g. `"params": "{...}"`) instead of an object (`"params": { ... }`), causing Joi validation and `execQuery` to receive the wrong shape. Please ensure `params` is sent as a JSON object, e.g. by parsing it (`payload["params"] = nlohmann::json::parse(params);`) or constructing the JSON object directly.
</issue_to_address>
### Comment 2
<location path="core/server/DatabaseAPI.cpp" line_range="3946" />
<code_context>
a_qry_end = a_qry_end;
a_qry_filter = a_qry_filter;
-
+ a_params = "{" + a_params + "}";
return cnt;
}
</code_context>
<issue_to_address>
**question:** Wrapping `a_params` with braces at this layer risks double-wrapping or inconsistent formatting depending on how callers construct `a_params`.
Previously, this function left `a_params` untouched and `generalSearch` added the braces. Now the braces are added here while `generalSearch` passes `params` through. This only works if all callers always pass a raw fragment (e.g. `"cnt:10, offset:0"`) and never include braces.
If any caller already passes a braced string, this will yield `"{{...}}"` and break parsing. Consider standardizing `a_params` (always raw fragment vs. always full JSON) and ensuring that exactly one well-defined layer converts it to a JSON object (potentially by using `nlohmann::json` instead of string concatenation). Also double-check existing call sites for any that already include braces.
</issue_to_address>
### Comment 3
<location path="tests/end-to-end/test_api_query.py" line_range="31-38" />
<code_context>
+ + "python/datafed_pkg"
+ )
+ sys.path.insert(0, path_to_python_datafed_module)
+ try:
+ from datafed.CommandLib import API
+ except ImportError:
+ print(
+ "datafed was not found, make sure you are running script with "
+ "PYTHONPATH set to the location of the package in the datafed repo"
+ )
+ sys.exit(1)
+
+ from datafed import version as df_ver
</code_context>
<issue_to_address>
**suggestion (testing):** Prefer skipping tests instead of calling sys.exit when prerequisites (modules/env vars) are missing
Using `sys.exit(1)` in `setUp` will terminate the entire test run and surface as a hard failure instead of a skipped test due to missing prerequisites. Instead, call `self.skipTest(...)` when `datafed` (or other required modules) isn’t importable or required env vars like `DATAFED_DOMAIN` are unset, so the test is clearly marked as skipped and doesn’t block other tests from running.
</issue_to_address>
### Comment 4
<location path="CMakeLists.txt" line_range="37" />
<code_context>
building with static depencies is not completely possible because some system
</code_context>
<issue_to_address>
**issue (typo):** Fix spelling of "dependencies" and possessive "its" in this option description.
In this description, please change "depencies" to "dependencies" and "build it's libraries" to "build its libraries" (possessive).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| payload["qry_end"] = qry_end; | ||
| payload["qry_filter"] = qry_filter; | ||
| payload["params"] = "{" + params + "}"; | ||
| payload["params"] = params; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The params field is still sent as a string while the Foxx API now expects a JSON object, which can break validation and query execution.
On the Foxx side, params is now validated as joi.object() and is no longer JSON.parsed from a string, but here payload["params"] is still assigned from a std::string. That will serialize as a JSON string value (e.g. "params": "{...}") instead of an object ("params": { ... }), causing Joi validation and execQuery to receive the wrong shape. Please ensure params is sent as a JSON object, e.g. by parsing it (payload["params"] = nlohmann::json::parse(params);) or constructing the JSON object directly.
| a_qry_end = a_qry_end; | ||
| a_qry_filter = a_qry_filter; | ||
|
|
||
| a_params = "{" + a_params + "}"; |
Contributor
There was a problem hiding this comment.
question: Wrapping a_params with braces at this layer risks double-wrapping or inconsistent formatting depending on how callers construct a_params.
Previously, this function left a_params untouched and generalSearch added the braces. Now the braces are added here while generalSearch passes params through. This only works if all callers always pass a raw fragment (e.g. "cnt:10, offset:0") and never include braces.
If any caller already passes a braced string, this will yield "{{...}}" and break parsing. Consider standardizing a_params (always raw fragment vs. always full JSON) and ensuring that exactly one well-defined layer converts it to a JSON object (potentially by using nlohmann::json instead of string concatenation). Also double-check existing call sites for any that already include braces.
Comment on lines
+31
to
+38
| try: | ||
| from datafed.CommandLib import API | ||
| except ImportError: | ||
| print( | ||
| "datafed was not found, make sure you are running script with " | ||
| "PYTHONPATH set to the location of the package in the datafed repo" | ||
| ) | ||
| sys.exit(1) |
Contributor
There was a problem hiding this comment.
suggestion (testing): Prefer skipping tests instead of calling sys.exit when prerequisites (modules/env vars) are missing
Using sys.exit(1) in setUp will terminate the entire test run and surface as a hard failure instead of a skipped test due to missing prerequisites. Instead, call self.skipTest(...) when datafed (or other required modules) isn’t importable or required env vars like DATAFED_DOMAIN are unset, so the test is clearly marked as skipped and doesn’t block other tests from running.
| @@ -36,7 +36,7 @@ building with static depencies is not completely possible because some system | |||
| libraries must be shared libraries for DataFed to be interoperable. If this | |||
| setting is turned on DataFed will build it's libraries as shared and try to | |||
Contributor
There was a problem hiding this comment.
issue (typo): Fix spelling of "dependencies" and possessive "its" in this option description.
In this description, please change "depencies" to "dependencies" and "build it's libraries" to "build its libraries" (possessive).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Update Staging
Summary by Sourcery
Bump project versions to the 2.x release line, refine query parameter handling, and expand end-to-end testing and documentation for web and query functionality.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: