Skip to content

Conversation

rschmitt
Copy link
Contributor

This change rewrites the equals and hashCode implementations in SortedArrayStringMap and JdkMapAdapterStringMap to support value-based equality between the two implementations.

Fixes #3669.

Copy link

github-actions bot commented May 19, 2025

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@vy vy self-assigned this May 19, 2025
@vy
Copy link
Member

vy commented May 19, 2025

Wouldn't it be better to

  1. ReadOnlyStringMapUtil internal class such that
    • It contains a static equals(ReadOnlyStringMap,ReadOnlyStringMap) method
    • It contains a hashCode(ReadOnlyStringMap) method
  2. Use ReadOnlyStringMapUtil to implement equals and hashCode in all ReadOnlyStringMap implementations

@rschmitt
Copy link
Contributor Author

@vy OK

@rschmitt
Copy link
Contributor Author

What should I do about these baseline failures?

@vy
Copy link
Member

vy commented May 22, 2025

@rschmitt, bump the @Version tag to 2.25.0 in package-info.java files located where you touched files. (Just answering your question, next week I will review your PR in detail.)

@vy vy moved this from To triage to In review in Log4j bug tracker May 22, 2025
Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone for tackling this pull request!

Just to add my perspective: while ReadOnlyStringMapUtil initially seemed like a solid solution, introducing it creates a binary incompatibility between log4j-core 2.25.x and log4j-api 2.24.x. Since the main goal of #3669 is to support unit testing, it might be preferable to avoid that incompatibility and take a simpler—albeit slightly less efficient—approach by using ReadOnlyStringMap.toMap() within the equals() and hashCode() implementations.

This change rewrites the `equals` and `hashCode` implementations
throughout the `ReadOnlyStringMap` hierarchy to use a common
implementation that supports value-based equality between
implementations.

Fixes apache#3669.
@ppkarwasz ppkarwasz merged commit 49cdf74 into apache:2.x Jun 2, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Log4j bug tracker Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Different SimpleMap implementations are never equal

3 participants