davsclaus commented on PR #25889:
URL: https://github.com/apache/camel/pull/25889#issuecomment-5467422965
Thanks for the thorough review! Addressed below:
1. **`buildTypeSchema` defensive type check** — good catch, applied.
Replaced the fallthrough `(Class<?>) type` cast with `if (!(type instanceof
Class<?> clazz)) { return new JsonObject(); }`, so a
`TypeVariable`/`WildcardType`/`GenericArrayType` now degrades to an
unconstrained schema instead of throwing. Added
`GenerateDevConsoleMojoResponseSchemaTest.typeVariableFieldBuildsUnconstrainedSchemaInsteadOfThrowing`,
which uses a generic `Response<T>(T value)` record to exercise the branch
directly.
2. **`JsonRecordSupport.setAccessible(true)`** — kept this one, and I want
to explain why rather than just decline silently. I tested the actual scenario:
```java
// pkga.Holder (a different top-level class/package than the caller)
private record Priv(String name) {}
// pkgb.Accessor, reflecting on Priv's accessor from outside pkga
```
Invoking `Priv`'s accessor without `setAccessible(true)` throws:
```
IllegalAccessException: class pkgb.Accessor cannot access a member of
class pkga.Holder$Priv with modifiers "public"
```
So while the accessor method itself is always `public` per the record
spec, that doesn't override the accessibility restriction imposed by a
**non-public declaring class** when invoked reflectively from a different
package. Every current `Response` record happens to be `public`, so removing it
wouldn't break anything *today*, but `JsonRecordSupport` is meant to be
generic, reusable infrastructure — not scoped to "public records only" — and
every console's record lives in a different package than `JsonRecordSupport`
itself. I'd rather keep the defensive call than trade correctness for cosmetic
cleanup here.
3. **Missing enum test coverage** — added
`JsonRecordSupportTest.enumFieldConvertsToItsName`, covering an enum-typed
record component end to end (`Status.ACTIVE` → `"ACTIVE"`).
4. **`includeProcessorsJSon` signature change** — acknowledged, no action
needed; as noted, it's in the `o.a.c.impl.console` implementation package and
already called out in the PR description.
On the PR metadata suggestions (labels/milestone) — per this project's merge
procedure, those are typically set by the merging committer at merge time
rather than by the author beforehand, so I'll leave that for whoever merges.
_Claude Sonnet 5 on behalf of @davsclaus_
--
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]
