Repository navigation
fix(admin): remove the unsupported YAPI import source - #7946
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Request changes: historical api_source = 4 rows become read-only
The cleanup itself is right — IMPORT_YAPI is not implemented, advertising it in the enum, the DTO javadoc, and the dashboard only invites people to select a source that cannot work. But tightening @Max(4) to @Max(3) on a DTO that is shared by create and update locks existing rows out of the edit path, and the PR body confirms those rows are intentionally left in place.
The problem
ApiDTO.apiSource is validated on both endpoints:
// ApiController
@PostMapping
public ShenyuAdminResult createApi(@Valid @RequestBody final ApiDTO apiDTO)
@PutMapping("/{id}")
public ShenyuAdminResult updateApi(@PathVariable("id") ..., @Valid @RequestBody final ApiDTO apiDTO)With @NotNull plus @Max(3), an existing row that was created through the YAPI import path has exactly two outcomes on edit:
- dashboard sends the value it read back (
4) →Request error! invalid argument [apiSource: apiSource max 3] - dashboard omits the field to avoid that →
Request error! invalid argument [apiSource: apiSource not null]
There is no third option, so any API previously imported from YAPI can no longer be renamed, re-described, re-stated, or re-owned. ApiVO has no validation, so the rows still list and detail fine — the failure only appears when a user tries to save, which makes it worse: the record looks healthy until someone edits it.
The body states "Existing codes retain their values; historical database rows are not rewritten." That is a deliberate choice, but it is only safe if the write path tolerates the retained value. Right now it does not.
Suggested fixes, in order of preference
1. Stop accepting apiSource on update. The source is provenance — it should not change after creation. Either mark it read-only for deserialization and keep the stored value in the service, or reject a mismatch explicitly. This removes the class of problem rather than widening a bound.
2. Validate only on create. Move the @Min/@Max pair onto a validation group applied by createApi alone, and have updateApi ignore the field. This keeps the create-time guarantee the new test asserts while leaving existing rows editable.
3. Rewrite the rows. UPDATE api SET api_source = 2 WHERE api_source = 4 in the upgrade scripts, documented in RELEASE-NOTES as losing the original provenance. This contradicts the current stated intent, so only take it if you decide provenance is not worth preserving.
Widening back to @Max(4) would also unblock edits, but it re-advertises a source the code cannot produce, which is what this PR set out to remove.
Test gap
testCreateApiRejectsUnsupportedSource covers POST with 4 and correctly asserts verifyNoInteractions(apiService). There is no equivalent for PUT — neither "an existing row with api_source = 4 can still be updated" nor the decision to make it fail. Whichever way you resolve this, that case needs a test, because it is the behaviour that will be exercised by real deployments on upgrade.
Two smaller notes
- The packaged dashboard change is a hand edit of
index.16dee401.js. The body is upfront about this ("no full dashboard rebuild is claimed") and a companion change is going toapache/shenyu-dashboard, which is the right long-term home. Just be aware the bundled asset now differs from any published dashboard build, so the next rebuild will silently drop this edit unless the dashboard change lands first. Worth sequencing the two. @Min/@Maxmessage text was fixed from"state min 0"/"state max 4"to"apiSource ...". Good catch — those messages were pointing at the wrong field — but it means the new test asserts on a string that previously did not exist, so nothing else could have been relying on it. No action needed, just confirming the change is safe.
I will re-review as soon as the update path is decided.
Aias00
left a comment
There was a problem hiding this comment.
Review: #7946 fix(admin): remove the unsupported YAPI import source (re-review)
Verdict: approved — cleanly resolves the edit lock on historical records by separating creation validation from update persistence, and preserves API provenance across updates.
What was verified in this update
- Validation Group on Creation (
ApiDTO.Create):ApiDTOintroducespublic interface Create extends jakarta.validation.groups.Default {}.@NotNull,@Min(0), and@Max(3)onapiSourceare scoped strictly togroups = Create.class.ApiController.createApibinds@Validated(ApiDTO.Create.class), preventing new records from using the retired YAPI source (4).updateApiretains default validation without theapiSourceconstraint, allowing existing historical rows to be edited without validation rejections.
- Provenance Preservation on Update:
- In
ApiServiceImpl.update,apiDO.setApiSource(null)guarantees thatupdateByPrimaryKeySelectiveignoresapiSource, preserving the original database value even if the frontend passes back a modified or omitted source.
- In
- Test Coverage:
ApiControllerTest.testUpdateApiAcceptsHistoricalOrOmittedSource: verifies bothnulland4are accepted byPUT /api/{id}.ApiMapperTest.testUpdateHistoricalApiRetainsProvenance: asserts that updating an existing record withapi_source = 4successfully modifies description/owner/state while retainingapi_source = 4in the database.
- CI & Integration Test Alignments:
- Commits
5587ca12and02cf9cbbalso cleanly alignCryptorResponsePluginTestwith the expected-116error status, resolving the upstream test regression.
- Commits
Fixes #7594.
Remove IMPORT_YAPI and its advertised choice in the packaged dashboard. API creation accepts supported source codes 0–3. API source is immutable creation provenance: updates ignore the submitted source and retain the stored value, so historical source-4 records remain editable without rewriting their provenance.
Creation uses a validation group that also includes the common field constraints; updates retain common validation. Controller regressions cover supported creation, rejection of source 4, and updates with historical or omitted source. An H2 service/mapper regression starts from a source-4 row and verifies that edits to its description, owner and state preserve source 4 when the client submits null, 2 or 4.
Validation: 40 ApiControllerTest, ApiServiceTest and ApiMapperTest cases pass on Microsoft OpenJDK 17.0.18, including Checkstyle and affected-reactor Apache RAT. The packaged JavaScript edit only removes the unsupported choice; no full dashboard rebuild is claimed. Merge companion source change apache/shenyu-dashboard#728 before the next dashboard asset rebuild.
Shared CI fixture corrections: CryptorResponsePluginTest now checks the post-#7903 configuration error for both encrypt/decrypt when a field is absent, including absence of the original user data. SDK tests wait up to 60 seconds for usable registered routes before exercising assertions. ZooKeeper registration tests keep their declared backend ports reachable so the real health checker cannot race registry-removal assertions. Validation: 16 cryptor unit cases and 16 Admin discovery/health cases pass; the HTTP and SDK integration test reactors compile with Checkstyle. Full Docker integration execution is not claimed: the local Linux Docker engine is unavailable. Remote check results are tracked separately.