Skip to content

Define bulletin write fields explicitly - #4292

Merged
Duansg merged 4 commits into
apache:masterfrom
zqr10159:maintenance/bulletin-write-boundary
Aug 5, 2026
Merged

Define bulletin write fields explicitly#4292
Duansg merged 4 commits into
apache:masterfrom
zqr10159:maintenance/bulletin-write-boundary

Conversation

@zqr10159

Copy link
Copy Markdown
Member

Summary

  • create bulletins from an explicit set of business fields
  • update an existing bulletin in place while retaining persistence-managed fields
  • mark ids and audit metadata as read-only in the API schema

Validation

  • ./mvnw -pl hertzbeat-manager -am -Dtest=BulletinServiceTest test -DskipITs -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false
  • git diff --check

AI assistance: used for draft implementation and test iteration.
Human validation: all 7 focused bulletin service tests passed across the 22-module source reactor, with Checkstyle and diff hygiene passing.
Risk notes: create and edit now accept only the existing business fields; persistence-managed id and audit values are no longer copied from request entities.

@zqr10159
zqr10159 marked this pull request as ready for review July 31, 2026 02:51
@Duansg

Duansg commented Aug 3, 2026

Copy link
Copy Markdown
Member

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": , "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.

@zqr10159

zqr10159 commented Aug 4, 2026

Copy link
Copy Markdown
Member Author

Addressed the review findings in commit 68b340d4b0.

Changes:

  • create and update now use operation-specific name validation;
  • create rejects an existing name regardless of a caller-supplied ID;
  • update requires an ID and permits only the same record to retain its name;
  • added a database unique constraint as the final concurrent-write boundary.

Human validation:

  • BulletinServiceTest,BulletinControllerTest (11 tests passed);
  • git diff --check.

The new-head backend, Maven E2E, license, and label checks are currently queued by GitHub and have not started yet.

AI assistance: used for draft implementation and test iteration.
Risk notes: deployments with pre-existing duplicate bulletin names must clean those rows before schema validation can succeed.

@Duansg, please re-review this head when convenient.

@zqr10159
zqr10159 requested a review from Duansg August 4, 2026 13:20
@Duansg
Duansg merged commit bf3bb4f into apache:master Aug 5, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants