Duansg commented on PR #4292:
URL: https://github.com/apache/hertzbeat/pull/4292#issuecomment-5162760229
The underlying problem is real: addBulletin previously called
bulletinDao.save(bulletin) with a client-supplied id, which makes JPA merge
into an existing row — so POST /api/bulletin could overwrite an arbitrary
bulletin. Copying an explicit field set is the right shape of fix. One blocking
issue though.
The add path can now create duplicate bulletin names
validate() still consults the request's id when checking for duplicates
(BulletinServiceImpl.java:81-84):
Bulletin existBulletin = bulletinDao.findByName(bulletin.getName());
if (existBulletin != null &&
!existBulletin.getId().equals(bulletin.getId())) {
throw new IllegalArgumentException("Bulletin name duplicated");
}
BulletinController.java:65-66 runs validate() before addBulletin(). So a
POST carrying an existing bulletin's id and its name passes validation (the ids
match), and addBulletin then discards the id and inserts a second row with the
same name.
Before this PR the same request merged into the existing row, so no
duplicate was possible.
Bulletin.name has no unique constraint, and BulletinDao.java:38 declares
Bulletin findByName(String name) returning a single entity — once duplicates
exist, every subsequent add/edit touching that name throws
IncorrectResultSizeDataAccessException, and deleteByNameIn (line 33) removes
both rows.
Repro:
POST /api/bulletin {"id": <existing id>, "name": "<that bulletin's name>",
"app": "...", "monitorIds": [...], "fields": {...}}
then check select name, count(*) from hzb_bulletin group by name having
count(*) > 1.
Suggestion: ignore the request id when validating the create path (compare
against null, or split validation into create/update variants), and consider
adding a unique constraint on name so the invariant is enforced by the database
rather than by a read-then-write check that is racy anyway.
Note on the @Schema changes
Switching id / creator / modifier / gmtCreate / gmtUpdate to accessMode =
READ_ONLY only changes the generated OpenAPI document — Jackson still binds
those fields from the request body at runtime. The actual enforcement here is
the explicit field copy in addBulletin/editBulletin, which is fine, but the
annotations shouldn't be read as a control. Might be worth saying so in the PR
description so reviewers don't assume the schema is doing the work.
Optional
Since Bulletin is used as both the request body and the response body, the
explicit field list has to be kept in sync by hand — adding a business field
later and forgetting to add it to both methods fails silently. A small request
DTO would make that a compile-time concern instead.
--
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]
