spring-cloud-gateway icon indicating copy to clipboard operation
spring-cloud-gateway copied to clipboard

Identical Routes are not considered equal: AsyncPredicate does not override equals()

Open NadChel opened this issue 1 year ago • 6 comments

Two logically equal Routes are not programmatically equal since Route's equals() compares predicates, and the AsyncPredicate class does not override equals() (hence you get a simple reference comparison). Here's a test that reproduces the issue:

class RouteEqualityTest {

    @Test
    void testTwoIdenticalRouteAreEqual() {
        assertThat(getRouteBuilderStub().build()).isEqualTo(getRouteBuilderStub().build());
    }

    private Route.AsyncBuilder getRouteBuilderStub() {
        return Route.async()
                .id("123")
                .uri("https://example.com")
                .predicate(exchange -> exchange.getRequest()
                        .getPath()
                        .value()
                        .equals("/test-path"));
    }
}

NadChel avatar Jan 25 '24 01:01 NadChel

Does this cause problems for you?

spencergibb avatar Mar 11 '24 19:03 spencergibb

It does in tests. I found a workaround by creating my own PathOnlyAsyncPredicate that calls equals() between path strings, but it's, well, a workaround. It doesn't feel right that Route has equals() that is in effect no different from ==

NadChel avatar Mar 11 '24 20:03 NadChel

It boils down to the fact that we can't meaningfully compare lambdas since we can't compare method implementations. That is, we can compare state, not behavior. We can:

  1. Make AsyncPredicate implementations extend some AbstractAsyncPredicate instead of implementing AsyncPredicate directly. AbstractAsyncPredicate would have have an overridden equals() that delegates to its delegate Predicate, for example GatewayPredicate (this.delegate.equals(that.delegate))
  2. Introduce stateful OOB GatewayPredicates and encourage their use (and introduce state in already existing implementations). For example, PathRoutePredicateFactory.apply(Config) could return some PathGatewayPredicate instead of an anonymous subclass. PathGatewayPredicate would have a constructor accepting a collection of path patterns and equals() that compares the contents of those collections
  3. Explicitly state in the documentation to Route's equals() that it only meaningfully compares two Route instances if stateful Predicates are injected, otherwise it in effect compares references (generally, Spring Cloud Gateway has substantially underdelivered on javadocs)

@spencergibb are you on board with the plan? If so, I can put forward a PR

NadChel avatar Mar 20 '24 05:03 NadChel

Also, I'd like to know where I can ask some set-up questions. I read the README, looked for a community forum, to no avail. Specifically, I'm curious where org.springframework.cloud:spring-cloud-build:pom:4.1.1-SNAPSHOT is supposed to be pulled from (Maven build fails). Should I manually add it as a jar? It's not in Maven Central (not that version). It's unlikely you expect contributors to define custom repositories in poms

NadChel avatar Mar 20 '24 08:03 NadChel

https://repo.spring.io/snapshot is the snapshot repo. See https://start.spring.io/#!type=maven-project&language=java&platformVersion=3.3.0-SNAPSHOT&packaging=jar&jvmVersion=17&groupId=com.example&artifactId=demo&name=demo&description=Demo%20project%20for%20Spring%20Boot&packageName=com.example.demo for a maven example.

For 1) there are already implementations of AsyncPredicate that implement toString() for example, adding equals() there makes sense.

For 2) GatewayPredicate implements HasConfig which is the state. I wonder if a default equals() in GatewayPredicate that calls getConfig().

WDYT?

spencergibb avatar Mar 20 '24 15:03 spencergibb

Thank you for your assistance! I appreciate your help. Before we move on (and before I address your points concerning this particular issue), I'd like to ask you: are there good reasons why these repositories (they indeed contain necessary artifacts, I defined them in the pom, and Maven build succeeded)

  1. aren't already in the pom;
  2. are not mentioned in the README or CONTRIBUTING?

If there aren't any, I suggest we do at least one of the above to make onboarding of new contributors easier and, as a result, make the backlog of issues related to this project shrink even faster

	<repositories>
		<repository>
			<id>spring-milestones</id>
			<name>Spring Milestones</name>
			<url>https://repo.spring.io/milestone</url>
			<snapshots>
				<enabled>false</enabled>
			</snapshots>
		</repository>
		<repository>
			<id>spring-snapshots</id>
			<name>Spring Snapshots</name>
			<url>https://repo.spring.io/snapshot</url>
			<releases>
				<enabled>false</enabled>
			</releases>
		</repository>
	</repositories>
	<pluginRepositories>
		<pluginRepository>
			<id>spring-milestones</id>
			<name>Spring Milestones</name>
			<url>https://repo.spring.io/milestone</url>
			<snapshots>
				<enabled>false</enabled>
			</snapshots>
		</pluginRepository>
		<pluginRepository>
			<id>spring-snapshots</id>
			<name>Spring Snapshots</name>
			<url>https://repo.spring.io/snapshot</url>
			<releases>
				<enabled>false</enabled>
			</releases>
		</pluginRepository>
	</pluginRepositories>

NadChel avatar Mar 21 '24 14:03 NadChel