[cstamas]

Fixed (with regression tests, each confirmed to fail without its fix):

1. Entity equals/hashCode infinite recursion — Extension, Namespace, NamespaceMembership, UserData, PersonalAccessToken now compare/hash back-reference fields by id (mirroring ExtensionVersion's existing pattern) or omit mapped-by collections entirely (mirroring ExtensionScan's existing pattern), instead of hashing the full cyclic object graph. Verified with EntityEqualsHashCodeRecursionTest — reproduced the actual StackOverflowError against the old code.

2. LocalStorageService.uploadNamespaceLogo — now creates filePath.getParent() instead of filePath itself. Correction to my earlier writeup: the first upload for a namespace happened to succeed even with the bug (Java's Files.copy is allowed to replace an empty directory); it's specifically re-uploading a logo that threw FileAlreadyExistsException, since by then the path holds a regular file. LocalStorageServiceTest covers both the first upload and the replace case.

3. CustomerService — null-checks IPAddressString.getAddress() before calling .toIPv4() in both getCustomerByIpAddress and rebuildIPAddressCache; a malformed CIDR block for one customer now gets skipped with a warning log instead of aborting the whole cache rebuild. Covered by two new CustomerServiceTest cases.

4. FixMissingFilesJobRequestHandler — the "no vsix package available" guard now also fires when the DOWNLOAD FileResource row is absent entirely, not just when it's present-but-empty. Covered by FixMissingFilesJobRequestHandlerTest.

[Copilot]

🟢 Approval recommended

The fixes are targeted, align with existing entity id-based equality patterns in the codebase, and are covered by regression tests for each reported failure mode.

Pull request overview

Fixes several runtime bugs in the server module by hardening entity equals/hashCode implementations against cyclic JPA graphs, and by adding defensive handling for edge cases in storage, IP parsing, and migration jobs—each backed by new regression tests.

Changes:

• Prevent StackOverflowError in equals/hashCode by comparing back-references via entity id and excluding mapped-by collections from hashing.
• Fix LocalStorageService.uploadNamespaceLogo to create the parent directory (not the target file path).
• Make CustomerService resilient to malformed IP / CIDR inputs and ensure the missing-files migration job gracefully handles absent DOWNLOAD rows.

File summaries

File	Description
server/src/main/java/org/eclipse/openvsx/storage/LocalStorageService.java	Fix directory creation to target the logo file’s parent directory.
server/src/main/java/org/eclipse/openvsx/ratelimit/CustomerService.java	Null-check parsed IP addresses; skip malformed CIDR blocks with a warning instead of failing.
server/src/main/java/org/eclipse/openvsx/migration/FixMissingFilesJobRequestHandler.java	Treat a missing DOWNLOAD FileResource row as “no vsix available” and exit cleanly.
server/src/main/java/org/eclipse/openvsx/entities/UserData.java	Exclude cyclic collections from equals/hashCode to prevent recursion.
server/src/main/java/org/eclipse/openvsx/entities/PersonalAccessToken.java	Compare/hash the back-reference user by id to avoid recursion.
server/src/main/java/org/eclipse/openvsx/entities/NamespaceMembership.java	Compare/hash namespace and user back-references by id to avoid recursion.
server/src/main/java/org/eclipse/openvsx/entities/Namespace.java	Exclude cyclic collections from equals/hashCode to prevent recursion.
server/src/main/java/org/eclipse/openvsx/entities/Extension.java	Compare/hash namespace back-reference by id to avoid recursion.
server/src/test/java/org/eclipse/openvsx/storage/LocalStorageServiceTest.java	Regression tests for initial and replacement namespace logo uploads.
server/src/test/java/org/eclipse/openvsx/ratelimit/CustomerServiceTest.java	Regression tests for malformed IP input and malformed CIDR blocks.
server/src/test/java/org/eclipse/openvsx/migration/FixMissingFilesJobRequestHandlerTest.java	Regression test for missing DOWNLOAD row handling.
server/src/test/java/org/eclipse/openvsx/entities/EntityEqualsHashCodeRecursionTest.java	Regression test ensuring hashing does not recurse across bidirectional associations.

Review details

• Files reviewed: 12/12 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[netomi]

Reviewed this in depth - it looks sound and I would merge it. Four real bugs, correctly fixed, with
tests that genuinely bite. One finding I would act on before or shortly after merging, one worth
deciding deliberately, and some context that raises the priority of fix 3.

Verification

Rather than take the "confirmed to fail without its fix" claim on trust, I checked the branch out
into a worktree, reverted only the eight production files to the merge base (411c6ac64), and
re-ran the tests: 7 of the 9 fail. The two that pass are exactly the ones that should - the
pre-existing testGetCustomerByIpAddress, and uploadNamespaceLogo_storesTheFileUnderTheNamespaceLogoDirectory,
which the comment in the test already flags as passing even with the bug. The StackOverflowError
is real, reproduced at NamespaceMembership.hashCode:101. Full suite on the branch: 1225 tests, no
failures.

Extension.equals keeping versions is safe from recursion, because ExtensionVersion already
compares its back-references by id (ExtensionVersion.java:630). The description is accurate on
that point.

Worth crediting something the description undersells: switching namespace/user to id comparison
also fixes cross-hydration equality. NamespaceMembershipJooqRepository:47 builds a Namespace
with only an id set, which under the old field-by-field compare could never match a fully loaded
one.

1. Extension#versions should come out of equals/hashCode as well

It is the last mapped-by collection left in these five entities - Namespace and UserData both
had theirs removed here, citing ExtensionScan. Two consequences:

• @OneToMany(mappedBy = "extension") is LAZY, and Objects.hash(..., versions, ...) initialises
  the Hibernate bag. So every affectedExtensions.add(extension) in ExtensionService:257,
  AdminService:514 and PublisherComplianceChecker:93 - the very call paths this PR is fixing -
  issues a SELECT for the extension's entire version list purely to compute a hash, and equals
  then walks those lists element-wise. On a detached Extension, hashCode() throws
  LazyInitializationException.
• It is also a correctness wart. The jOOQ repositories hydrate new Extension() in ten places
  (ExtensionJooqRepository, ExtensionVersionJooqRepository, FileResourceJooqRepository) and
  never populate versions - there is no setVersions on the entity at all. So a jOOQ-hydrated and
  a Hibernate-managed Extension for the same row can never be equal.

Not introduced here, but it sits in the middle of what this PR is fixing.

2. Unassigned ids now compare equal

Every id is a primitive long, so an unpersisted entity has id 0 and getId(x) returns 0. Two
distinct new NamespaceMemberships with the same role are now equal (id == 0, both namespace
ids 0, both user ids 0, same role) where previously they compared the real objects.
server/AGENTS.md puts it directly: "Compare ids instead - and reject an unassigned id, or an
unpersisted entity makes the check fail open." This PR takes the first half of that sentence and not
the second. I could not find a call path that puts unpersisted instances of these types into a hash
collection, so nothing breaks today - but ExtensionVersion set the precedent without the guard, so
this is a good moment to decide either way on purpose.

3. The CustomerService NPE is remotely triggerable - worth saying so

IdentityService.getIPAddress:121 evaluates the configured ovsx.rate-limit.ip-address-function
against the request. The code default is getRemoteAddr(), always a literal - but the expression
demonstrated in server/src/dev/resources/application.yml:167 reads X-Forwarded-For. In any
deployment using that shape, a client sending a malformed header value reached
new IPAddressString(...).getAddress() returning null, i.e. an NPE on the rate-limiting path. That
makes this a remotely triggerable 500 rather than hardening, which is worth stating in the
description so it is weighted correctly at release time.

Nits

• The two new test files disagree on license header style: EntityEqualsHashCodeRecursionTest uses
  the /****... form that recent files use, LocalStorageServiceTest the older /** ****... one.
• missingFileTypes.stream().anyMatch(t -> t.equals(FileResource.DOWNLOAD)) is
  missingFileTypes.contains(...), and putting the cheap download.isEmpty() first reads better.
  Both disjuncts are needed though - DOWNLOAD can only enter missingFileTypes via the
  isFileMissing path, so neither subsumes the other.
• Explanation style is uneven: Namespace and UserData get a class-level comment, the other three
  an inline // use id to prevent infinite recursion.

Context: why do these entities have equals/hashCode at all

While reviewing I went looking for what consumes them, because that decides whether the fix should
be field-by-field patching or something more general. The answer is: almost nothing.

• They arrived in b778f848f (2022, "Performance improvements"), the commit that introduced ehcache
  and @Cacheable, so cache keys look like the original motivation.
• The caches do not use them. Every @Cacheable either takes no arguments or plain Strings, or
  supplies a custom key generator - and all four generators build a String out of namespace and
  extension names. The one bare @Cacheable that looks like a counter-example,
  ExtensionReviewRepository.averageRatingAndActiveTrue(), takes no arguments, so its key is
  SimpleKey.EMPTY.
• Hibernate does not need them. The classic reason is a Set-typed association; there is not a
  single private Set<...> in any entity, every association is a List, and within a session
  Hibernate guarantees one instance per row.
• What actually consumes them: the five LinkedHashSet<Extension|UserData> dedup sites, plus
  Mockito argument matching and equality assertions in the tests. All of those compare against the
  same instance, so identity semantics would satisfy every one of them.
• And 22 of the 40 entity classes have equals/hashCode while 18 do not - including FileResource
  and ExtensionReview - with no ill effect.

So the recursion bug class comes from field-by-field equality on entities that nobody actually
compares by field. Id-based equality with a constant per-class hashCode, which AGENTS.md already
recommends, would remove the class wholesale rather than one field at a time. Three cautions if
anyone picks that up as a follow-up: leave the value-ish classes alone (SemanticVersion,
Customer, Tier, Setting); changing equals changes Mockito stub matching and could quietly
weaken assertions that currently double as "was this built correctly" checks; and re-read the issue
989 context first, since AGENTS.md records UserData#equals having been load-bearing once.

None of that blocks this PR - it argues for finding 1, and for a follow-up issue.

Reviewed with Claude Code.