justinmclean commented on issue #8203:
URL: https://github.com/apache/gravitino/issues/8203#issuecomment-3212890241
I suggest you take a look at the test, passing a null TagInfo results in an
RTE when it tries tagInfo.name() in the CreateTagFailureEvent constructor.
```
public CreateTagFailureEvent(String user, String metalake, TagInfo
tagInfo, Exception exception) {
super(user, NameIdentifierUtil.ofTag(metalake, tagInfo.name()),
exception);
this.tagInfo = tagInfo;
}
```
So the question is could TagInfo be null? In the current code base, that
seems unlikely. But even if today’s code path “shouldn’t” pass null, adding a
small, local null-guard is cheap insurance that saves you from harder-to-debug
NPEs later.
Here’s why I think it’s still worth it:
- Public/protected methods are part of an API. You can’t guarantee every
caller validates inputs (especially as code evolves or gets reused). A fast,
explicit guard fails early with a clear message instead of a deep NPE. We’ve
had real NPEs slip in before (e.g., UserContext parentContext, CLI paths, Topic
properties).
- Fail-fast, clearer errors. Preconditions.checkArgument(x != null, "...")
or Objects.requireNonNull(x, "...") tells you what was wrong, not just a stack
trace. This makes incidents and bug triage much faster.
- Static analysis & documentation. The null guard documents the contract and
keeps tools like ErrorProne/SpotBugs/NullAway happy, preventing regressions
during reviews and refactors.
--
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]
