jerryshao commented on code in PR #10257:
URL: https://github.com/apache/gravitino/pull/10257#discussion_r2893596000
##########
core/src/main/java/org/apache/gravitino/job/BuiltInJobTemplateEventListener.java:
##########
@@ -310,7 +310,7 @@ private void registerNewBuiltInJobTemplate(String metalake,
JobTemplate jobTempl
private void updateBuiltInJobTemplate(
String metalake, JobTemplateEntity existing, JobTemplate newTemplate) {
- NameIdentifier identifier = NameIdentifierUtil.ofJobTemplate(metalake,
newTemplate.name());
+ NameIdentifier identifier = NameIdentifierUtil.ofJobTemplate(metalake,
existing.name());
Review Comment:
**Fix is semantically correct, but the bug cannot be triggered through the
production code path.**
`loadBuiltInJobTemplates()` always inserts with `template.name()` as the map
key:
```java
builtInTemplates.put(template.name(), template); // line 185
```
So in `reconcileBuiltInJobTemplates`, the loop's `name` key always equals
`newTemplate.name()`. The lookup `existingBuiltIns.remove(name)` therefore
always returns an entity whose `entity.name() == newTemplate.name()`. Both
expressions resolve to the same value at runtime.
Using `existing.name()` is still the right idiom — you have a reference to
the entity, so its identifier should come from the entity itself rather than
the incoming payload. It's a good defensive improvement.
Please add a comment or link the related issue (#10176?) explaining a
concrete scenario where `existing.name()` and `newTemplate.name()` can differ,
so future readers understand why this guard is needed.
--
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]
