diff --git a/server/AGENTS.md b/server/AGENTS.md new file mode 100644 index 000000000..48f6b4885 --- /dev/null +++ b/server/AGENTS.md @@ -0,0 +1,150 @@ +# AGENTS.md + +Operating rules for AI coding agents working on the Open VSX registry server — +the Spring Boot application in `src/main/java`, built with Gradle. Claude Code +loads this file via `CLAUDE.md`; other agents read it directly. + +This is the short, must-follow contract. If any rule here conflicts with an +explicit request from the user, ask before overriding it. + +## Persistence: `EntityManager.merge` needs justifying, every time + +`merge` copies a **whole detached entity** over the stored row. Any column that +changed since that entity was loaded is silently reverted. It is a write, even +where the surrounding code only means to read. + +Never reach for `merge` to obtain a managed entity. Use +`entityManager.find(Type.class, id)` and set the fields you mean to change — +the setters are what persist inside a transaction. + +Before writing or keeping a `merge`, answer all five: + +1. **Is an update actually intended here?** If the method only reads, or only + needs the entity managed so a lazy association loads, use `find`. +2. **Is the entity detached?** An already-managed entity makes `merge` a no-op + that returns the same instance — the call is noise. +3. **How stale is it?** An entity loaded before slow work (VSIX parsing, a + remote call) and merged afterwards will revert anything a concurrent writer + changed in between, `active` included. +4. **Does anything depend on instance identity?** `UserData#equals` compares + every field, so a `merge` can be load-bearing purely by making two + references `==`. Compare ids instead — and reject an unassigned id, or an + unpersisted entity makes the check fail open. +5. **Is the row still there?** `find` returns `null` for a deleted row where + `merge` would try to resurrect it. Handle the `null`. + +`merge` before `remove` works but emits a pointless UPDATE ahead of the DELETE. + +This is issue #989, reported as a concurrent write to an extension being +reverted on a *read* path, leaving extensions inactive. Treat a new `merge` as +something to argue for in the pull request description. + +## Non-negotiables + +- **`./gradlew test` must pass before you commit.** Some tests start + Testcontainers (Postgres, Elasticsearch, LocalStack), so a working Docker + daemon is required; say so rather than skipping them silently. +- **`./gradlew spotlessCheck` must pass for the files you touched.** It is + deliberately not wired into `build`/`check` (`enforceCheck = false`), and the + repository has pre-existing violations elsewhere — so run `spotlessApply` and + then **revert files you did not otherwise change**, rather than sweeping + unrelated formatting into your commit. +- **New source files need the EPL-2.0 license header** (copy it from any + existing file). +- **Never commit unless the user asks**, and stage only the files you changed + (`git add `), never `git add -A` / `git add .`. + +## Project shape + +- **Java 25** (`libs.versions.toml`). Dependencies are declared in + `gradle/libs.versions.toml`, never inline in `build.gradle`. +- **Nullability is expressed with JSpecify** (`@Nullable`, `@NonNull` from + `org.jspecify.annotations`), not Jakarta or Spring annotations. +- **`src/main/jooq-gen/` is generated and committed.** Never hand-edit it; it + is excluded from Spotless. Regenerate it with `./gradlew jooqCodegen` after a + schema change — that task reads a live Postgres, so it needs the dev database + running. +- **Flyway migrations** live in `src/main/resources/db/migration` as + `V__Description.sql`. A migration that has shipped is immutable — fix a + mistake with a new one. They are excluded from the pre-commit hooks. +- **Configuration properties** are bound in `*Config` classes with `@Value`, + each documented with its property name and default, and validated in a + `@PostConstruct`. A property with an invalid value should fail startup rather + than misbehave later. +- The server has **no CHANGELOG** — only `cli/` and `webui/` do. Do not invent + one; put the reasoning in the commit message and pull request instead. + +## Deployment descriptors travel with the config + +A property added, renamed or removed in the server has **four** homes that can +drift apart: + +- `src/dev/resources/application.yml` +- `../deploy/docker/configuration/application.yml` +- `../deploy/openshift/application.yml` +- `../deploy/kubernetes/configmap.yaml` (the same document, indented inside + `data:`) + +The Kubernetes one was added later than the others and has twice been missed by +changes that updated the rest, leaving keys that silently bind to nothing. When +you touch configuration, diff all four and say which you changed. + +## Changing code requires tests + +- JUnit 5 with Mockito and AssertJ under `src/test/java`, mirroring the source + package. +- For a bug fix, **confirm the test fails without the fix** — a regression test + that passes either way is not one. +- Prefer a focused unit test over booting the whole context. A config class can + be exercised with `ApplicationContextRunner`; note that a bare runner has no + conversion service, so register one + (`ApplicationConversionService.getSharedInstance()`) or `Duration` and + collection properties will not bind. +- A change with no matching test update is incomplete. Say so explicitly rather + than skipping silently. + +## Workflow and code quality + +- Read files in full before wide-ranging changes, and before editing files you + have not inspected. Do not rely on search snippets. +- Keep code comments short (1–3 lines): state only the non-obvious constraint or + rationale, never narrate what the code does. +- Ask before removing functionality or code that appears intentional. Do not + preserve backward compatibility unless the user asks for it. +- A configuration property that has never appeared in a release can be renamed + outright; one that has shipped needs a fallback to its old key (see + `ovsx.access-token.prefix`). Check the tags before assuming either. + +## Conventions + +- Commit subjects use a conventional-commit prefix: `feat:`, `fix:`, `chore:`, + `docs:`, `test:`, `style:`, `build:`, or `ci:`. +- No emojis in commits, pull requests, issues, or code. Keep prose concise, + direct, and technical — no cheerful filler. +- Answer a user's question before making edits or running implementation + commands. +- When responding to feedback or an analysis, say whether you agree or disagree + before describing what you changed. + +## Git + +Multiple sessions may be running in this cwd at the same time, each modifying +different files. Git operations that touch unstaged, staged, or untracked files +outside your own changes will stomp on other sessions' work. Follow these rules: + +Committing: + +- Only commit files YOU changed in THIS session. +- Stage explicit paths (`git add `); never `git add -A` / `git add .`. +- Before committing, run `git status` and verify you are only staging your files. + +Never run (destroys other agents' work or bypasses checks): + +- `git reset --hard`, `git checkout .`, `git clean -fd`, `git stash`, + `git add -A`, `git add .`, `git commit --no-verify`. + +If rebase conflicts occur: + +- Resolve conflicts only in files you modified. +- If a conflict is in a file you did not modify, abort and ask the user. +- Never force push. diff --git a/server/CLAUDE.md b/server/CLAUDE.md new file mode 120000 index 000000000..47dc3e3d8 --- /dev/null +++ b/server/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java index a5ee0d7e8..a3b821f6f 100644 --- a/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java +++ b/server/src/main/java/org/eclipse/openvsx/accesstoken/AccessTokenService.java @@ -255,13 +255,16 @@ public ResultJson deactivateAccessToken(UserData user, long id) { throw new NotFoundException(); } - user = entityManager.merge(user); - if (!token.getUser().equals(user)) { + // Compare ids, not entities: UserData#equals compares every field - tokens and memberships + // included - so comparing entities rejects a caller whose user differs from the stored row in + // any way. Rejecting id 0 stops an entity that was never persisted from failing the check open. + var tokenUser = token.getUser(); + if (tokenUser == null || tokenUser.getId() == 0 || tokenUser.getId() != user.getId()) { throw new NotFoundException(); } token.setActive(false); - return ResultJson.success("Deactivated access token for user " + user.getLoginName() + "."); + return ResultJson.success("Deactivated access token for user " + tokenUser.getLoginName() + "."); } // REQUIRES_NEW: callers such as LocalRegistryService#createNamespace(NamespaceJson, String) wrap diff --git a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java index a123b37f9..78dad75f5 100644 --- a/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java +++ b/server/src/main/java/org/eclipse/openvsx/admin/ChangeNamespaceService.java @@ -80,7 +80,9 @@ public void changeNamespaceInDatabase( private void changeExtensionNamespace(Streamable extensions, Namespace newNamespace) { for (var extension : extensions) { - extension = entityManager.merge(extension); + // findExtensions ran inside changeNamespaceInDatabase's transaction, so these are + // already managed and merge just handed back the same instance; the setter is what + // persists the change. extension.setNamespace(newNamespace); } } diff --git a/server/src/main/java/org/eclipse/openvsx/eclipse/PublisherComplianceChecker.java b/server/src/main/java/org/eclipse/openvsx/eclipse/PublisherComplianceChecker.java index 68fbba8ac..6bc68d3a5 100644 --- a/server/src/main/java/org/eclipse/openvsx/eclipse/PublisherComplianceChecker.java +++ b/server/src/main/java/org/eclipse/openvsx/eclipse/PublisherComplianceChecker.java @@ -12,7 +12,6 @@ import java.util.LinkedHashSet; import java.util.Optional; -import jakarta.persistence.EntityManager; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Value; @@ -35,7 +34,6 @@ public class PublisherComplianceChecker { protected final Logger logger = LoggerFactory.getLogger(PublisherComplianceChecker.class); private final TransactionTemplate transactions; - private final EntityManager entityManager; private final RepositoryService repositories; private final ExtensionService extensions; private final EclipseService eclipseService; @@ -45,13 +43,11 @@ public class PublisherComplianceChecker { public PublisherComplianceChecker( TransactionTemplate transactions, - EntityManager entityManager, RepositoryService repositories, ExtensionService extensions, EclipseService eclipseService ) { this.transactions = transactions; - this.entityManager = entityManager; this.repositories = repositories; this.extensions = extensions; this.eclipseService = eclipseService; @@ -102,7 +98,6 @@ private void deactivateExtensions(UserData user) { // the version stops being publicly visible here, which the changes feed reports at // this instant rather than at the one it was published at repositories.recordExtensionVersionChange(version, ExtensionVersionState.INACTIVE, now); - entityManager.merge(version); var extension = version.getExtension(); affectedExtensions.add(extension); logger.atInfo() @@ -114,8 +109,10 @@ private void deactivateExtensions(UserData user) { // Update affected extensions for (var extension : affectedExtensions) { + // findVersionsByUser runs inside checkPublishers' TransactionTemplate, so these + // entities (and the versions above) are already managed and merge only handed back the + // same instances; the setters are what persist. extensions.updateExtension(extension); - entityManager.merge(extension); } } } diff --git a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java index 3bb0bd145..d3c14a150 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/MigrationService.java @@ -86,7 +86,17 @@ public MigrationService( */ @Transactional public void scheduleMigration(MigrationItem item, Instant scheduledAt) { - item = entityManager.merge(item); + // find rather than merge: only migrationScheduled below is this method's to write, and + // merging the whole detached item reverted anything else that had moved - see #989. + var itemId = item.getId(); + item = entityManager.find(MigrationItem.class, itemId); + if (item == null) { + // The row can be deleted between the caller reading its batch and this transaction + // (MigrationItemCleanupFilter, or one of the Delete_MigrationItems migrations). Skip it + // rather than dereferencing null, which would abort the rest of the batch. + logger.debug("Migration item {} is gone, nothing to schedule", itemId); + return; + } var jobIdText = item.getJobName() + "->itemId=" + item.getId(); var jobId = uuidService.generateFromName(jobIdText); var handler = JOB_HANDLERS.get(item.getJobName()); diff --git a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java index 7b0d13f04..93e41b23c 100644 --- a/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java +++ b/server/src/main/java/org/eclipse/openvsx/migration/RenameDownloadsService.java @@ -26,7 +26,15 @@ public RenameDownloadsService(EntityManager entityManager) { @Transactional public FileResource cloneResource(FileResource resource, String name) { - resource = entityManager.merge(resource); + // Nothing on `resource` is updated here; find still returns a managed entity, so the lazy + // getExtension() below resolves just the same - see #989. + var resourceId = resource.getId(); + resource = entityManager.find(FileResource.class, resourceId); + if (resource == null) { + // Deleted since the job loaded it. The caller uses the clone straight away, so there is + // nothing to skip - fail with the id rather than an NPE on the next line. + throw new IllegalStateException("Cannot clone file resource " + resourceId + ": it no longer exists"); + } var clone = new FileResource(); clone.setName(name); clone.setStorageType(resource.getStorageType()); diff --git a/server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java b/server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java index 0742de673..efe93d226 100644 --- a/server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java +++ b/server/src/main/java/org/eclipse/openvsx/storage/log/DownloadCountProcessor.java @@ -111,9 +111,16 @@ public List increaseDownloadCounts(Map extensionDownlo @Transactional // needs transaction for lazy-loading versions public void evictCaches(Extension extension) { Observation.createNotStarted("DownloadCountProcessor#evictCaches", observations).observe(() -> { - var mergedExtension = entityManager.merge(extension); - cache.evictExtensionJsons(mergedExtension); - cache.evictLatestExtensionVersion(mergedExtension); + // find, not merge: this only needs a managed entity so the lazy version collections + // can be loaded for eviction, and nothing here updates the extension. merge wrote the + // whole (already mutated, possibly stale) row back, which is how a concurrent update + // could be silently reverted - the failure mode behind #989. + var managedExtension = entityManager.find(Extension.class, extension.getId()); + if (managedExtension == null) { + return; + } + cache.evictExtensionJsons(managedExtension); + cache.evictLatestExtensionVersion(managedExtension); }); } diff --git a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java index 106ff249f..bee995847 100644 --- a/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java +++ b/server/src/main/java/org/eclipse/openvsx/trustedpublishing/TrustedPublishingService.java @@ -168,7 +168,15 @@ public TrustedPublisher registerTrustedPublisher( publisher.setProvider(provider.getProviderId()); publisher.setRegistration(registration); publisher.setClaims(claims); - publisher.setCreatedBy(entityManager.merge(user)); + // find, not merge: this needs a managed UserData to point the new row at, and merging the + // caller's (detached) user wrote their whole row back as a side effect - see #989. + // trusted_publisher.created_by is NOT NULL, so a user that has gone away since the request + // was authenticated has to fail here rather than as a constraint violation on persist. + var createdBy = entityManager.find(UserData.class, user.getId()); + if (createdBy == null) { + throw new ErrorResultException("Cannot register a trusted publisher for an unknown user."); + } + publisher.setCreatedBy(createdBy); publisher.setCreatedTimestamp(TimeUtil.getCurrentUTC()); entityManager.persist(publisher); return publisher; diff --git a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java index 185700a26..6bfb9bf5b 100644 --- a/server/src/test/java/org/eclipse/openvsx/UserAPITest.java +++ b/server/src/test/java/org/eclipse/openvsx/UserAPITest.java @@ -216,8 +216,6 @@ void testDeleteAccessToken() throws Exception { token.setType(PersonalAccessTokenType.LLT); Mockito.when(repositories.findPersonalAccessToken(100)) .thenReturn(token); - Mockito.when(entityManager.merge(userData)) - .thenReturn(userData); mockMvc.perform( post("/user/token/delete/{id}", 100) @@ -258,6 +256,7 @@ void testDeleteAccessTokenInactive() throws Exception { void testDeleteAccessTokenWrongUser() throws Exception { mockUserData(); var userData = new UserData(); + userData.setId(2); userData.setLoginName("wrong_user"); var token = new PersonalAccessToken(); token.setId(100); @@ -923,6 +922,7 @@ void testDeleteDependingExtension() throws Exception { private UserData mockUserData() { var userData = new UserData(); + userData.setId(1); userData.setLoginName("test_user"); userData.setFullName("Test User"); userData.setProviderUrl("http://example.com/test"); diff --git a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java index 5dfdfdbda..bb3ea55ef 100644 --- a/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/accesstoken/AccessTokenServiceTest.java @@ -37,8 +37,10 @@ import org.eclipse.openvsx.entities.UserData; import org.eclipse.openvsx.mail.MailService; import org.eclipse.openvsx.repositories.RepositoryService; +import org.eclipse.openvsx.util.NotFoundException; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.lenient; @@ -83,6 +85,66 @@ private PersonalAccessToken activeUnrestrictedToken() { return token; } + // #989: deactivateAccessToken used to call entityManager.merge(user) so that `user` became the + // same managed instance as token.getUser() and UserData#equals - which compares every field, + // tokens and memberships included - could short-circuit on ==. The merge wrote the caller's + // whole user row back as a side effect. These pin down that the ownership check now goes by id, + // so it neither writes through the EntityManager nor depends on the caller's copy of the user + // matching the stored row field for field. + @Test + void deactivatesATokenForItsOwnerWithoutWritingTheUserBack() { + var owner = new UserData(); + owner.setId(1L); + owner.setLoginName("owner"); + var token = activeUnrestrictedToken(); + token.setUser(owner); + when(repositories.findPersonalAccessToken(7L)).thenReturn(token); + + // The caller's copy carries stale fields, exactly what merge would have written back. + var stale = new UserData(); + stale.setId(1L); + stale.setLoginName("owner"); + stale.setFullName("a stale full name"); + + var result = accessTokenService.deactivateAccessToken(stale, 7L); + + assertThat(result.getError()).isNull(); + assertThat(token.isActive()).isFalse(); + verifyNoInteractions(entityManager); + } + + @Test + void refusesToDeactivateATokenBelongingToSomebodyElse() { + var owner = new UserData(); + owner.setId(1L); + var token = activeUnrestrictedToken(); + token.setUser(owner); + when(repositories.findPersonalAccessToken(7L)).thenReturn(token); + + var other = new UserData(); + other.setId(2L); + + assertThatThrownBy(() -> accessTokenService.deactivateAccessToken(other, 7L)) + .isInstanceOf(NotFoundException.class); + assertThat(token.isActive()).isTrue(); + } + + // personal_access_token.user_data is nullable, so an ownerless row must not be deactivatable by + // whoever asks - and must not NPE on the way to refusing. + @Test + void refusesToDeactivateATokenWithNoUser() { + var token = activeUnrestrictedToken(); + token.setUser(null); + when(repositories.findPersonalAccessToken(7L)).thenReturn(token); + + var user = new UserData(); + user.setId(1L); + + assertThatThrownBy(() -> accessTokenService.deactivateAccessToken(user, 7L)) + .isInstanceOf(NotFoundException.class); + assertThat(token.isActive()).isTrue(); + } + @Test void authenticatesWithTheTokensUser() { var user = new UserData();