[DAPS-1830] - core foxx refactored json schema integration 1#1892
Merged
JoshuaSBrown merged 15 commits intodevelfrom Mar 24, 2026
Merged
[DAPS-1830] - core foxx refactored json schema integration 1#1892JoshuaSBrown merged 15 commits intodevelfrom
JoshuaSBrown merged 15 commits intodevelfrom
Conversation
Contributor
Reviewer's GuideRefactors schema validation and storage in the core server by extracting responsibilities into dedicated SchemaHandler, validator, storage, and factory components, updates ClientWorker to use SchemaHandler, and adds comprehensive unit and integration tests plus build wiring. Sequence diagram for schema create request handling via SchemaHandlersequenceDiagram
actor Client
participant ClientWorker
participant SchemaHandler
participant DatabaseAPI
participant JsonLib as NlohmannJSON
participant JsonValidator as JsonSchemaValidatorLib
Client->>ClientWorker: send SchemaCreateRequest
ClientWorker->>ClientWorker: PROC_MSG_BEGIN macro expands
ClientWorker->>SchemaHandler: handleCreate(a_uid, request, reply, log_context)
SchemaHandler->>DatabaseAPI: setClient(a_uid)
SchemaHandler->>SchemaHandler: validateSchemaDefinition(request.def, log_context)
activate SchemaHandler
SchemaHandler->>JsonLib: parse(def) -> schema
SchemaHandler->>SchemaHandler: enforceRequiredProperties(schema)
SchemaHandler->>JsonValidator: create json_validator with schemaLoader callback
SchemaHandler->>JsonValidator: set_root_schema(schema)
deactivate SchemaHandler
alt validation ok
SchemaHandler->>DatabaseAPI: schemaCreate(request, log_context)
DatabaseAPI-->>SchemaHandler: ack
SchemaHandler-->>ClientWorker: return
ClientWorker->>ClientWorker: PROC_MSG_END macro expands
ClientWorker-->>Client: AckReply
else validation error
SchemaHandler-->>ClientWorker: throw TraceException("Invalid metadata schema")
ClientWorker-->>Client: error propagated
end
Class diagram for new schema validation and storage componentsclassDiagram
direction LR
class ClientWorker {
-DatabaseAPI m_db_client
-SchemaHandler* m_schema_handler
+procSchemaCreateRequest(a_uid, msg_request, log_context) : unique_ptr~IMessage~
+procSchemaReviseRequest(a_uid, msg_request, log_context) : unique_ptr~IMessage~
+procSchemaUpdateRequest(a_uid, msg_request, log_context) : unique_ptr~IMessage~
+procMetadataValidateRequest(a_uid, msg_request, log_context) : unique_ptr~IMessage~
}
class SchemaHandler {
-DatabaseAPI& m_db_client
+SchemaHandler(a_db_client)
+handleCreate(a_uid, a_request, a_reply, log_context) : void
+handleRevise(a_uid, a_request, a_reply, log_context) : void
+handleUpdate(a_uid, a_request, a_reply, log_context) : void
+handleMetadataValidate(a_uid, a_request, a_reply, log_context) : void
+schemaLoader(a_uri, a_value, log_context) : void
+enforceRequiredProperties(a_schema) static : void
-validateSchemaDefinition(a_def, log_context) : void
}
class ISchemaValidator {
<<interface>>
+validateDefinition(a_schema_format, a_content, log_context) : ValidationResult
+validateMetadata(a_schema_id, a_metadata_format, a_metadata_content, log_context) : ValidationResult
+hasValidationCapability() : bool
+cacheSchema(a_schema_id, a_content, a_format, log_context) : bool
+evictSchema(a_schema_id) : void
}
class JsonSchemaValidator {
-SchemaLoaderCallback m_loader
-mutex m_loader_mutex
-LogContext m_current_log_context
-shared_mutex m_cache_mutex
-unordered_map~string, shared_ptr~json_validator~~ m_schema_cache
+JsonSchemaValidator(a_loader)
+setSchemaLoader(a_loader) : void
+validateDefinition(a_schema_format, a_content, log_context) : ValidationResult
+validateMetadata(a_schema_id, a_metadata_format, a_metadata_content, log_context) : ValidationResult
+hasValidationCapability() : bool
+cacheSchema(a_schema_id, a_content, a_format, log_context) : bool
+evictSchema(a_schema_id) : void
+clearCache() : void
+isCached(a_schema_id) : bool
-enforceDataFedRequirements(a_schema) : void
-schemaLoaderAdapter(a_uri, a_value) : void
-compileSchema(a_schema, log_context) : shared_ptr~json_validator~
}
class ExternalSchemaValidator {
-unique_ptr~SchemaAPIClient~ m_client
-string m_engine
+ExternalSchemaValidator(a_client, a_engine)
+validateDefinition(a_schema_format, a_content, log_context) : ValidationResult
+validateMetadata(a_schema_id, a_metadata_format, a_metadata_content, log_context) : ValidationResult
+hasValidationCapability() : bool
}
class NullSchemaValidator {
+NullSchemaValidator()
+validateDefinition(a_schema_format, a_content, log_context) : ValidationResult
+validateMetadata(a_schema_id, a_metadata_format, a_metadata_content, log_context) : ValidationResult
+hasValidationCapability() : bool
}
class ISchemaStorage {
<<interface>>
+storeContent(a_id, a_content, a_desc, log_context) : string
+retrieveContent(a_id, a_arango_def, log_context) : StorageRetrieveResult
+updateContent(a_id, a_content, a_desc, log_context) : string
+deleteContent(a_id, log_context) : void
}
class ArangoSchemaStorage {
+ArangoSchemaStorage()
+storeContent(a_id, a_content, a_desc, log_context) : string
+retrieveContent(a_id, a_arango_def, log_context) : StorageRetrieveResult
+updateContent(a_id, a_content, a_desc, log_context) : string
+deleteContent(a_id, log_context) : void
}
class ExternalSchemaStorage {
-unique_ptr~SchemaAPIClient~ m_client
+ExternalSchemaStorage(a_client)
+storeContent(a_id, a_content, a_desc, log_context) : string
+retrieveContent(a_id, a_arango_def, log_context) : StorageRetrieveResult
+updateContent(a_id, a_content, a_desc, log_context) : string
+deleteContent(a_id, log_context) : void
}
class SchemaAPIClient {
-SchemaAPIConfig m_config
-CURL* m_curl
+SchemaAPIClient(a_config)
+~SchemaAPIClient()
+isConfigured() : bool
+putSchema(a_id, a_name, a_description, a_content, log_context) : void
+patchSchema(a_id, a_name, a_description, a_content, log_context) : void
+getSchema(a_id, log_context) : json
+deleteSchema(a_id, log_context) : void
+validateSchema(a_schema_format, a_engine, a_content, a_errors, log_context) : bool
+validateMetadata(a_schema_id, a_metadata_format, a_engine, a_metadata_content, a_errors, a_warnings, log_context) : bool
-httpGet(a_path, log_context) : json
-httpPost(a_path, a_body, a_http_code, log_context) : json
-httpPut(a_path, a_body, log_context) : json
-httpPatch(a_path, a_body, log_context) : json
-httpDelete(a_path, log_context) : void
-curlPerform(a_method, a_url, a_body, a_http_code, log_context) : json
}
class SchemaAPIConfig {
+string base_url
+string bearer_token
+bool verify_ssl
+string ca_cert_path
+string client_cert_path
+string client_key_path
+long connect_timeout_sec
+long request_timeout_sec
+isConfigured() : bool
+hasAuth() : bool
}
class SchemaServiceFactory {
-shared_ptr~ISchemaStorage~ m_default_storage
-shared_ptr~ISchemaValidator~ m_default_validator
-unordered_map~string, shared_ptr~ISchemaStorage~~ m_storage
-unordered_map~string, shared_ptr~ISchemaValidator~~ m_validators
+setDefaultStorage(a_storage) : void
+registerStorage(a_engine, a_storage) : void
+getStorage(a_engine) : ISchemaStorage
+setDefaultValidator(a_validator) : void
+registerValidator(a_engine, a_validator) : void
+getValidator(a_engine) : ISchemaValidator
+hasCustomStorage(a_engine) : bool
+hasCustomValidator(a_engine) : bool
-resolveStorage(a_engine) : ISchemaStorage
-resolveValidator(a_engine) : ISchemaValidator
}
class ValidationResult {
+bool valid
+string errors
+string warnings
+Ok(a_warnings) static : ValidationResult
+Fail(a_errors) static : ValidationResult
}
class StorageRetrieveResult {
+bool success
+string content
+string error
+Ok(a_content) static : StorageRetrieveResult
+Fail(a_error) static : StorageRetrieveResult
}
%% Relationships
ClientWorker --> SchemaHandler : uses
SchemaHandler --> DatabaseAPI : uses
ISchemaValidator <|.. JsonSchemaValidator
ISchemaValidator <|.. ExternalSchemaValidator
ISchemaValidator <|.. NullSchemaValidator
ISchemaStorage <|.. ArangoSchemaStorage
ISchemaStorage <|.. ExternalSchemaStorage
ExternalSchemaStorage --> SchemaAPIClient : uses
ExternalSchemaValidator --> SchemaAPIClient : uses
SchemaAPIClient --> SchemaAPIConfig : has
SchemaServiceFactory --> ISchemaStorage : returns
SchemaServiceFactory --> ISchemaValidator : returns
SchemaServiceFactory o--> ISchemaStorage : holds
SchemaServiceFactory o--> ISchemaValidator : holds
File-Level Changes
Possibly linked issues
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 5 issues, and left some high level feedback:
- SchemaAPIClient currently ignores several fields in SchemaAPIConfig (notably bearer_token, client_cert_path, and client_key_path), so if these are expected to control authentication/TLS you should wire them into curlPerform (e.g., add an Authorization header and client certificate options) or explicitly document that they are unused.
- The DataFed-specific schema checks are implemented twice (JsonSchemaValidator::enforceDataFedRequirements and SchemaHandler::enforceRequiredProperties); consider centralizing this logic in a single helper to avoid future divergence in behavior between paths.
- ExternalSchemaStorage and ExternalSchemaValidator are documented as not thread-safe but there is no guard at the factory or call sites; consider enforcing one-instance-per-thread more explicitly (e.g., via construction patterns or comments where they are instantiated) to prevent accidental sharing across threads.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SchemaAPIClient currently ignores several fields in SchemaAPIConfig (notably bearer_token, client_cert_path, and client_key_path), so if these are expected to control authentication/TLS you should wire them into curlPerform (e.g., add an Authorization header and client certificate options) or explicitly document that they are unused.
- The DataFed-specific schema checks are implemented twice (JsonSchemaValidator::enforceDataFedRequirements and SchemaHandler::enforceRequiredProperties); consider centralizing this logic in a single helper to avoid future divergence in behavior between paths.
- ExternalSchemaStorage and ExternalSchemaValidator are documented as not thread-safe but there is no guard at the factory or call sites; consider enforcing one-instance-per-thread more explicitly (e.g., via construction patterns or comments where they are instantiated) to prevent accidental sharing across threads.
## Individual Comments
### Comment 1
<location path="core/server/schema_validators/JsonSchemaValidator.cpp" line_range="52-61" />
<code_context>
+ m_loader = std::move(a_loader);
+}
+
+void JsonSchemaValidator::schemaLoaderAdapter(const nlohmann::json_uri &a_uri,
+ nlohmann::json &a_value) {
+ // Extract schema ID from URI path (skip leading "/")
+ // This matches the existing ClientWorker::schemaLoader behavior
+ std::string id = a_uri.path();
+ if (!id.empty() && id[0] == '/') {
+ id = id.substr(1);
+ }
+
+ // m_loader_mutex must be held by caller (compileSchema)
+ DL_DEBUG(m_current_log_context,
+ "JsonSchemaValidator loading schema ref: " << id
+ << " (scheme=" << a_uri.scheme() << ", path=" << a_uri.path()
+ << ")");
+
+ if (!m_loader) {
+ throw std::runtime_error("Schema $ref resolution failed: no loader "
+ "configured for schema ID: " + id);
+ }
+
+ a_value = m_loader(id, m_current_log_context);
+
+ DL_TRACE(m_current_log_context, "Loaded referenced schema: " << a_value);
</code_context>
<issue_to_address>
**issue (bug_risk):** Schema loader adapter accesses shared state without locking, breaking the stated thread-safety guarantees
`schemaLoaderAdapter` assumes `m_loader_mutex` is held, but `compileSchema` only holds it during `set_root_schema`. When the returned validator later resolves `$ref`s (potentially from multiple threads), `schemaLoaderAdapter` is called without the mutex, so `m_loader` and `m_current_log_context` are accessed (and `m_loader` potentially modified via `setSchemaLoader`) without synchronization, leading to data races and UB.
To keep the validator thread-safe, either:
- Take `m_loader_mutex` inside `schemaLoaderAdapter` around all uses of `m_loader` and `m_current_log_context`, or
- Preferably, capture the loader and log context into the validator at construction time (e.g., via a lambda with copied state) so each compiled validator has an immutable, thread-safe loader callback.
</issue_to_address>
### Comment 2
<location path="core/server/SchemaAPIClient.cpp" line_range="64-73" />
<code_context>
+ EXCEPT(INTERNAL_ERROR,
+ "SchemaAPIClient: not configured (no base URL provided)");
+
+ std::string res_buf;
+ char error[CURL_ERROR_SIZE] = {};
+
+ curl_easy_setopt(m_curl, CURLOPT_URL, a_url.c_str());
+ curl_easy_setopt(m_curl, CURLOPT_WRITEDATA, &res_buf);
+ curl_easy_setopt(m_curl, CURLOPT_ERRORBUFFER, error);
+ curl_easy_setopt(m_curl, CURLOPT_CUSTOMREQUEST, a_method.c_str());
+
+ struct curl_slist *headers = nullptr;
+ headers = curl_slist_append(headers, "Content-Type: application/json");
+ headers = curl_slist_append(headers, "Accept: application/json");
+
+ std::string corr_header = "x-correlation-id: " + log_context.correlation_id;
+ headers = curl_slist_append(headers, corr_header.c_str());
+
+ curl_easy_setopt(m_curl, CURLOPT_HTTPHEADER, headers);
+
+ if (a_body && !a_body->empty()) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Authorization/bearer token from SchemaAPIConfig is not used in HTTP requests
`SchemaAPIConfig` exposes a `bearer_token`/`hasAuth()`, but `curlPerform` never sets an `Authorization` header. If the schema API requires auth, requests will fail even when the token is configured. Please wire this through, e.g.
```cpp
if (m_config.hasAuth()) {
std::string auth = "Authorization: Bearer " + m_config.bearer_token;
headers = curl_slist_append(headers, auth.c_str());
}
```
If `SchemaAPIConfig` also carries mTLS settings (client cert/key), consider applying those curl options here as well.
</issue_to_address>
### Comment 3
<location path="core/server/client_handlers/SchemaHandler.cpp" line_range="43-52" />
<code_context>
+
+// ── Private ─────────────────────────────────────────────────────────────────
+
+void SchemaHandler::validateSchemaDefinition(const std::string &a_def,
+ LogContext log_context) {
+ nlohmann::json schema = nlohmann::json::parse(a_def);
+
+ enforceRequiredProperties(schema);
+
+ nlohmann::json_schema::json_validator validator(
+ bind(&SchemaHandler::schemaLoader, this, placeholders::_1,
+ placeholders::_2, log_context));
+
+ validator.set_root_schema(schema);
+}
+
</code_context>
<issue_to_address>
**suggestion:** Schema validation logic is duplicated between SchemaHandler and JsonSchemaValidator
This sequence (parse JSON → enforce DataFed requirements → construct `json_validator` with `schemaLoader` → `set_root_schema`) essentially duplicates `JsonSchemaValidator::validateDefinition` / `compileSchema`. Duplicating this logic risks the two flows diverging over time (e.g., new rules/formats added in one place only). Consider having `SchemaHandler::validateSchemaDefinition` delegate to `JsonSchemaValidator` or a shared helper so validation behavior and error mapping are defined in a single location.
</issue_to_address>
### Comment 4
<location path="core/server/client_handlers/SchemaHandler.cpp" line_range="182-191" />
<code_context>
+
+ // Stack-local error handler — no shared mutable state.
+ // Replaces the old m_validator_err member on ClientWorker.
+ struct LocalErrorHandler
+ : nlohmann::json_schema::basic_error_handler {
+ std::string errors;
+ void error(const nlohmann::json::json_pointer &ptr,
+ const nlohmann::json &instance,
+ const std::string &message) override {
+ if (!errors.empty())
+ errors += "\n";
+ errors += "At " + ptr.to_string() + ": " + message;
+ basic_error_handler::error(ptr, instance, message);
+ }
+ } handler;
+
+ try {
</code_context>
<issue_to_address>
**suggestion:** LocalErrorHandler implementation differs from the reusable one in JsonSchemaValidator
There are now two similar but slightly different `LocalErrorHandler` implementations (here and in `JsonSchemaValidator.cpp`), which can cause inconsistent error messages and duplicated logic. Please extract a single shared implementation (e.g., a small header) and use it in both places, or route metadata validation through the existing `JsonSchemaValidator` abstraction instead of duplicating the pattern.
Suggested implementation:
```cpp
nlohmann::json_schema::json_validator validator(
bind(&SchemaHandler::schemaLoader, this, placeholders::_1,
placeholders::_2, log_context));
// Stack-local error handler — no shared mutable state.
// Reuses the shared implementation from JsonSchemaValidator to keep
// error formatting consistent across the server.
JsonSchema::LocalErrorHandler handler;
validateSchemaDefinition(a_request.def(), log_context);
```
To fully implement the “single shared LocalErrorHandler” suggestion, you’ll also need to:
1. **Extract the existing LocalErrorHandler from JsonSchemaValidator.cpp into a header**:
- Create a small header, for example `core/server/validation/JsonSchemaErrorHandler.h`, that contains the `LocalErrorHandler` definition used by `JsonSchemaValidator.cpp`.
- Place it in an appropriate namespace (e.g. `namespace JsonSchema { ... }`) so it can be reused here and elsewhere.
2. **Update JsonSchemaValidator.cpp**:
- Remove its local/anonymous-namespace `LocalErrorHandler` definition.
- Include the new header (`#include "JsonSchemaErrorHandler.h"`).
- Use the shared `JsonSchema::LocalErrorHandler` type.
3. **Include the new header in SchemaHandler.cpp and adjust namespace if needed**:
- Add `#include "JsonSchemaErrorHandler.h"` (or whatever name/path you choose) near the other includes.
- If the chosen namespace is different from `JsonSchema::LocalErrorHandler`, update the replacement above accordingly (e.g. `using JsonSchema::LocalErrorHandler;` or fully qualify the type used here).
4. **Wire the handler into the validator (if not already done elsewhere in the function)**:
- Ensure that after constructing `validator` and `handler`, you call `validator.set_error_handler(handler);` before validation, and that validation failures are surfaced consistently (e.g. via `handler.errors` or by rethrowing as in `JsonSchemaValidator`).
</issue_to_address>
### Comment 5
<location path="core/server/tests/integration/test_SchemaServiceFactory.cpp" line_range="60-69" />
<code_context>
+BOOST_AUTO_TEST_SUITE(FactoryWithJsonSchemaValidator)
</code_context>
<issue_to_address>
**suggestion (testing):** Integration tests could also cover interactions with external validators/storage when those are wired in
Given the new `ExternalSchemaValidator` and `ExternalSchemaStorage`, please extend this suite (or add a sibling one) configuring the factory with those components backed by a mock `SchemaAPIClient`. That would let you assert that:
- external validators are invoked via the factory,
- errors and warnings from the external API are reflected in `ValidationResult`, and
- fallback to default validators still works when external engines are misconfigured.
This will provide end-to-end coverage of the external-schema integration at the factory level.
Suggested implementation:
```cpp
BOOST_AUTO_TEST_CASE(register_json_schema_validator) {
SchemaServiceFactory factory;
auto json_validator = std::make_shared<JsonSchemaValidator>();
factory.registerValidator("JSONSchema", json_validator);
ISchemaValidator &retrieved = factory.getValidator("JSONSchema");
BOOST_TEST(retrieved.hasValidationCapability() == true);
}
BOOST_AUTO_TEST_CASE(external_validator_is_invoked_via_factory) {
// Arrange
SchemaServiceFactory factory;
auto mock_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(mock_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(mock_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{R"({"name": "Alice", "age": 42})"};
// The mock is expected to be called when validation is triggered via the factory
EXPECT_CALL(*mock_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([](const std::string &, const std::string &, ExternalValidationResponse &out) {
out.errors.clear();
out.warnings.clear();
return true;
});
// Act
auto &validator = factory.getValidator("ExternalJSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert
BOOST_TEST(result.isValid());
BOOST_TEST(result.errors().empty());
BOOST_TEST(result.warnings().empty());
}
BOOST_AUTO_TEST_CASE(external_validator_propagates_errors_and_warnings) {
// Arrange
SchemaServiceFactory factory;
auto mock_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(mock_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(mock_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{kInvalidUserSchema}; // uses the invalid JSON sample from anonymous namespace
ExternalValidationResponse response;
response.errors = {
ExternalValidationError{"type", "expected integer, got string"}};
response.warnings = {
ExternalValidationWarning{"age", "value is out of recommended range"}};
EXPECT_CALL(*mock_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([&response](const std::string &, const std::string &, ExternalValidationResponse &out) {
out = response;
return true;
});
// Act
auto &validator = factory.getValidator("ExternalJSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert
BOOST_TEST(!result.isValid());
BOOST_TEST(result.errors().size() == response.errors.size());
BOOST_TEST(result.warnings().size() == response.warnings.size());
BOOST_TEST(result.errors()[0].path == response.errors[0].path);
BOOST_TEST(result.errors()[0].message == response.errors[0].message);
BOOST_TEST(result.warnings()[0].path == response.warnings[0].path);
BOOST_TEST(result.warnings()[0].message == response.warnings[0].message);
}
BOOST_AUTO_TEST_CASE(fallback_to_default_validator_when_external_misconfigured) {
// Arrange
SchemaServiceFactory factory;
// Default JSON schema validator still registered
auto json_validator = std::make_shared<JsonSchemaValidator>();
factory.registerValidator("JSONSchema", json_validator);
// External pieces are present but misconfigured (e.g. API client throws / fails)
auto failing_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(failing_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(failing_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{R"({"name": "Alice", "age": 42})"};
EXPECT_CALL(*failing_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([](const std::string &, const std::string &, ExternalValidationResponse &) {
throw std::runtime_error("external validation engine misconfigured");
});
// Act: factory should transparently fall back to the default JSONSchema validator
ISchemaValidator &validator = factory.getValidator("JSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert: despite external failure, default validator still succeeds
BOOST_TEST(result.isValid());
BOOST_TEST(result.errors().empty());
}
```
The above tests assume several APIs and types that may differ from your actual implementation. To integrate them you will likely need to:
1. Adjust the mock type:
- If you already have a `SchemaAPIClient` interface, create `MockSchemaAPIClient` using your mocking framework (e.g., gmock) with a method compatible with how `ExternalSchemaValidator`/`ExternalSchemaStorage` call it (I used `validateSchema(schema_id, payload, ExternalValidationResponse&)` as a placeholder).
- Replace `EXPECT_CALL`/`testing::_` with your actual mocking constructs if you are not using gmock.
2. Align constructor and wiring:
- Update the construction of `ExternalSchemaValidator` and `ExternalSchemaStorage` to match their real constructors (they might take the client by reference, pointer, or additional configuration).
- Replace `factory.setSchemaStorage(...)` and `factory.registerValidator("ExternalJSONSchema", external_validator);` with the actual configuration methods on `SchemaServiceFactory` that wire external storage/validators.
3. Match `ValidationResult` API:
- Update `result.isValid()`, `result.errors()`, `result.warnings()` accessors to whatever your `ValidationResult` exposes (e.g., `ok()`, `getErrors()`, `getWarnings()`).
- Adjust field access for errors/warnings (`path`, `message`) to the fields your error/warning types actually have.
4. Use the correct invalid JSON payload:
- I referenced `kInvalidUserSchema` from the anonymous namespace; ensure this symbol exists or replace it with your actual invalid JSON sample defined at the top of the file.
5. If your design triggers validation only via a higher-level factory method (e.g. `factory.validate(schema_id, payload)`):
- Replace the direct `validator.validate(schema_id, payload)` calls with the appropriate factory-level entry point so the tests truly exercise “end-to-end” behavior through `SchemaServiceFactory`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+60
to
+69
| BOOST_AUTO_TEST_SUITE(FactoryWithJsonSchemaValidator) | ||
|
|
||
| BOOST_AUTO_TEST_CASE(register_json_schema_validator) { | ||
| SchemaServiceFactory factory; | ||
| auto json_validator = std::make_shared<JsonSchemaValidator>(); | ||
|
|
||
| factory.registerValidator("JSONSchema", json_validator); | ||
|
|
||
| ISchemaValidator &retrieved = factory.getValidator("JSONSchema"); | ||
| BOOST_TEST(retrieved.hasValidationCapability() == true); |
Contributor
There was a problem hiding this comment.
suggestion (testing): Integration tests could also cover interactions with external validators/storage when those are wired in
Given the new ExternalSchemaValidator and ExternalSchemaStorage, please extend this suite (or add a sibling one) configuring the factory with those components backed by a mock SchemaAPIClient. That would let you assert that:
- external validators are invoked via the factory,
- errors and warnings from the external API are reflected in
ValidationResult, and - fallback to default validators still works when external engines are misconfigured.
This will provide end-to-end coverage of the external-schema integration at the factory level.
Suggested implementation:
BOOST_AUTO_TEST_CASE(register_json_schema_validator) {
SchemaServiceFactory factory;
auto json_validator = std::make_shared<JsonSchemaValidator>();
factory.registerValidator("JSONSchema", json_validator);
ISchemaValidator &retrieved = factory.getValidator("JSONSchema");
BOOST_TEST(retrieved.hasValidationCapability() == true);
}
BOOST_AUTO_TEST_CASE(external_validator_is_invoked_via_factory) {
// Arrange
SchemaServiceFactory factory;
auto mock_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(mock_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(mock_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{R"({"name": "Alice", "age": 42})"};
// The mock is expected to be called when validation is triggered via the factory
EXPECT_CALL(*mock_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([](const std::string &, const std::string &, ExternalValidationResponse &out) {
out.errors.clear();
out.warnings.clear();
return true;
});
// Act
auto &validator = factory.getValidator("ExternalJSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert
BOOST_TEST(result.isValid());
BOOST_TEST(result.errors().empty());
BOOST_TEST(result.warnings().empty());
}
BOOST_AUTO_TEST_CASE(external_validator_propagates_errors_and_warnings) {
// Arrange
SchemaServiceFactory factory;
auto mock_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(mock_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(mock_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{kInvalidUserSchema}; // uses the invalid JSON sample from anonymous namespace
ExternalValidationResponse response;
response.errors = {
ExternalValidationError{"type", "expected integer, got string"}};
response.warnings = {
ExternalValidationWarning{"age", "value is out of recommended range"}};
EXPECT_CALL(*mock_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([&response](const std::string &, const std::string &, ExternalValidationResponse &out) {
out = response;
return true;
});
// Act
auto &validator = factory.getValidator("ExternalJSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert
BOOST_TEST(!result.isValid());
BOOST_TEST(result.errors().size() == response.errors.size());
BOOST_TEST(result.warnings().size() == response.warnings.size());
BOOST_TEST(result.errors()[0].path == response.errors[0].path);
BOOST_TEST(result.errors()[0].message == response.errors[0].message);
BOOST_TEST(result.warnings()[0].path == response.warnings[0].path);
BOOST_TEST(result.warnings()[0].message == response.warnings[0].message);
}
BOOST_AUTO_TEST_CASE(fallback_to_default_validator_when_external_misconfigured) {
// Arrange
SchemaServiceFactory factory;
// Default JSON schema validator still registered
auto json_validator = std::make_shared<JsonSchemaValidator>();
factory.registerValidator("JSONSchema", json_validator);
// External pieces are present but misconfigured (e.g. API client throws / fails)
auto failing_client = std::make_shared<MockSchemaAPIClient>();
auto external_storage = std::make_shared<ExternalSchemaStorage>(failing_client);
auto external_validator = std::make_shared<ExternalSchemaValidator>(failing_client);
factory.setSchemaStorage(external_storage);
factory.registerValidator("ExternalJSONSchema", external_validator);
const std::string schema_id{"user-schema"};
const std::string payload{R"({"name": "Alice", "age": 42})"};
EXPECT_CALL(*failing_client, validateSchema(schema_id, payload, testing::_))
.Times(1)
.WillOnce([](const std::string &, const std::string &, ExternalValidationResponse &) {
throw std::runtime_error("external validation engine misconfigured");
});
// Act: factory should transparently fall back to the default JSONSchema validator
ISchemaValidator &validator = factory.getValidator("JSONSchema");
ValidationResult result = validator.validate(schema_id, payload);
// Assert: despite external failure, default validator still succeeds
BOOST_TEST(result.isValid());
BOOST_TEST(result.errors().empty());
}
The above tests assume several APIs and types that may differ from your actual implementation. To integrate them you will likely need to:
- Adjust the mock type:
- If you already have a
SchemaAPIClientinterface, createMockSchemaAPIClientusing your mocking framework (e.g., gmock) with a method compatible with howExternalSchemaValidator/ExternalSchemaStoragecall it (I usedvalidateSchema(schema_id, payload, ExternalValidationResponse&)as a placeholder). - Replace
EXPECT_CALL/testing::_with your actual mocking constructs if you are not using gmock.
- If you already have a
- Align constructor and wiring:
- Update the construction of
ExternalSchemaValidatorandExternalSchemaStorageto match their real constructors (they might take the client by reference, pointer, or additional configuration). - Replace
factory.setSchemaStorage(...)andfactory.registerValidator("ExternalJSONSchema", external_validator);with the actual configuration methods onSchemaServiceFactorythat wire external storage/validators.
- Update the construction of
- Match
ValidationResultAPI:- Update
result.isValid(),result.errors(),result.warnings()accessors to whatever yourValidationResultexposes (e.g.,ok(),getErrors(),getWarnings()). - Adjust field access for errors/warnings (
path,message) to the fields your error/warning types actually have.
- Update
- Use the correct invalid JSON payload:
- I referenced
kInvalidUserSchemafrom the anonymous namespace; ensure this symbol exists or replace it with your actual invalid JSON sample defined at the top of the file.
- I referenced
- If your design triggers validation only via a higher-level factory method (e.g.
factory.validate(schema_id, payload)):- Replace the direct
validator.validate(schema_id, payload)calls with the appropriate factory-level entry point so the tests truly exercise “end-to-end” behavior throughSchemaServiceFactory.
- Replace the direct
JoshuaSBrown
added
Component: Core
JoshuaSBrown
added this to the
Create a LinkML Validator Service for DataFed Schema Support milestone
JoshuaSBrown
changed the title
nedvedba
approved these changes
Mar 23, 2026
Collaborator
nedvedba
left a comment
nedvedba
left a comment
There was a problem hiding this comment.
LGTM! A lot cleaner and better documented than what was there originally.
JoshuaSBrown
changed the title
Collaborator
Author
Thanks Blake! |
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
2 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.
Ticket
Description
How Has This Been Tested?
Artifacts (if appropriate):
Tasks
Summary by Sourcery
Introduce a pluggable schema validation and storage architecture, refactor schema handling out of ClientWorker into dedicated components, and add comprehensive unit and integration tests for the new schema services.
Bug Fixes:
Enhancements:
Build:
Tests: