OpenFeign / feign

Feign makes writing java http clients easier
Apache License 2.0
9.44k stars 1.92k forks source link

Backward compatibility issue of overload resolution ambiguity #2458

Open heli-os opened 3 months ago

heli-os commented 3 months ago

issue starts with https://github.com/OpenFeign/feign/pull/2170

Many code snippets are used this way.

RetryableException(
    response.status(),
    RATE_LIMIT_EXCEEDED,
    response.request().httpMethod(),
    null, // Date type (original)
    response.request(),
)

However, this code will not work with that change.

  1. get a compile time error
Overload resolution ambiguity: 
public constructor RetryableException(p0: Int, p1: String!, p2: Request.HttpMethod!, p3: Date!, p4: Request!) defined in feign.RetryableException
public constructor RetryableException(p0: Int, p1: String!, p2: Request.HttpMethod!, p3: Long!, p4: Request!) defined in feign.RetryableException
  1. ambiguous what to do when we don't want to use retry.

Two solutions to solve this problem

it's not intuitive, and it creates more confusion.

// first solution
RetryableException(
    response.status(),
    RATE_LIMIT_EXCEEDED,
    response.request().httpMethod(),
    null as Long?,
    response.request(),
)
// second solution
val nonRetryable: Long? = null
RetryableException(
    response.status(),
    RATE_LIMIT_EXCEEDED,
    response.request().httpMethod(),
    nonRetryable,
    response.request(),
)

Suggest two things

  1. fundamentally fix the overload resolution ambiguity.
    • need to ideation about how to fix it.
  2. annotate how it should be written in the new constructor when don't want to retry.
    • What about adding it to a new constructor?