Akanksha-kedia commented on PR #18896:
URL: https://github.com/apache/pinot/pull/18896#issuecomment-4855243168
## Code Review — PR #18896 (`fix/schema-utils-validation-message`)
The fix is correct — the exception message from `PinotHelixResourceManager`
(which constructs detailed incompatibility info including missing columns,
field-type changes, and suggestions) now flows through to the HTTP 400 response
body. Two **MAJOR** findings on test coverage, several **MINOR** style issues.
---
### C5 — Correctness & Nulls
**[MINOR] `e.getMessage()` null-safety**
(`PinotSchemaRestletResource.java:480`)
`SchemaBackwardIncompatibleException` always passes a non-null message at
its only throw site today. However, the constructor has no
`Objects.requireNonNull` guard — a future subclass or test stub could pass
`null`, producing `"Reason: null"`. Consider:
```java
String.format("Backward incompatible schema %s. Reason: %s", schemaName,
Objects.toString(e.getMessage(), "(no message)"))
```
**[MINOR] Asymmetry between `addSchema` and `updateSchema` error format**
`addSchema` (line 439) forwards `e.getMessage()` with no outer prefix.
`updateSchema` (line 477) now wraps it as `"Backward incompatible schema %s.
Reason: %s"`. A user hitting `POST /schemas?override=true` vs `PUT
/schemas/{name}` gets different message shapes for the same underlying error.
Also: the outer prefix `"Backward incompatible schema %s."` and the inner
message both identify the schema by name — the prefix is redundant.
---
### C6 — Testing
**[MAJOR] No message-content assertion for the `addSchema` (POST override)
backward-incompatibility path**
`createSchema` on line 108 still uses `expectValidationException`, which
only checks the exception type. Since `addSchema`'s catch block also passes
`e.getMessage()`, the detailed-message contract for that path is untested. Add
an `expectValidationExceptionWithMessage` assertion there too.
**[MINOR] Asserting on implementation-internal substring `"Incompatible
field specifications"`**
This string is constructed in `PinotHelixResourceManager`. If the wording
changes, the test becomes a weaker assertion silently. A more stable anchor is
`"Reason:"` (owned by the REST layer) or the column name being changed.
---
### C3 — Naming & API
**[MINOR] `"Reason:"` prefix usage is inconsistent across the file**
`validateSchemaInternal` uses concatenation (`". Reason: "`);
`updateSchema`'s generic `Exception` catch and the new handler use format
strings (`"Reason: %s"`); `addSchema`'s handler uses no prefix at all.
Standardise on one form.
---
### C8 — Process & Scope
**[MAJOR] Scope gap: only `updateSchema` was changed, but the test does not
validate the pre-existing `addSchema` message path**
`addSchema` (line 439) was already forwarding `e.getMessage()` correctly
before this PR. The PR correctly fixes only `updateSchema`. However, there is
no test confirming `addSchema` also surfaces the detail. The PR title says
"include validation failure reason" — adding the `addSchema` test would make
coverage complete.
---
### C1 — Architecture (advisory)
**[MINOR] Detailed incompatibility message is constructed in the resource
manager, not in the exception or a value type**
The 60-line error-building block in
`PinotHelixResourceManager.updateSchema()` (lines 1626–1685) is user-facing
text embedded inside the manager. A cleaner long-term direction:
`Schema.isBackwardCompatibleWith()` returns a structured
`BackwardCompatibilityResult` (list of violations) and the REST layer does the
formatting. This PR doesn't make things worse, but flagging for future
consideration.
---
### Pre-existing issue (not introduced here)
`updateSchema` returns `SuccessResponse(schema.getSchemaName() + "
successfully added")` (line 472) — should be `"successfully updated"`. Worth a
follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
