Skip to content

Backward compatibility issue of overload resolution ambiguity #2458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
heli-os opened this issue Jun 26, 2024 · 0 comments · May be fixed by #2871
Open

Backward compatibility issue of overload resolution ambiguity #2458

heli-os opened this issue Jun 26, 2024 · 0 comments · May be fixed by #2871
Labels
good first issue Issues that are good for first time contributors to tackle help wanted Issues we need help with tackling proposal Proposed Specification or API change

Comments

@heli-os
Copy link
Contributor

heli-os commented Jun 26, 2024 •
edited
Loading

issue starts with #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.
  • Just looking at the code in the IDE didn't work, so I came to repository.

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?
@heli-os heli-os mentioned this issue Jun 26, 2024
Merged
@kdavisk6 kdavisk6 added help wanted Issues we need help with tackling good first issue Issues that are good for first time contributors to tackle proposal Proposed Specification or API change labels Sep 11, 2024
rojae added a commit to rojae/feign that referenced this issue Apr 27, 2025
@rojae
Add constructor to RetryableException for cases without retryAfter
0cbcadc 
Added two new constructors to RetryableException:
- A constructor without retryAfter and without cause
- A constructor without retryAfter but with cause

These new constructors remove overload ambiguity
when retryAfter is not needed.

Added a unit test to verify the new constructors.
- canCreateRetryableExceptionWithoutRetryAfter
- canCreateRetryableExceptionWithoutRetryAfterAndWithCause

Fixes OpenFeign#2458
@rojae rojae linked a pull request Apr 27, 2025 that will close this issue
Open
rojae added a commit to rojae/feign that referenced this issue Apr 27, 2025
@rojae
Add constructor to RetryableException for cases without retryAfter
31d0395 
Added two new constructors to RetryableException:
- A constructor without retryAfter and without cause
- A constructor without retryAfter but with cause

Standardized JavaDoc across all RetryableException constructors

These new constructors remove overload ambiguity when retryAfter is not needed.

Added a unit test to verify the new constructors.
- canCreateRetryableExceptionWithoutRetryAfter
- canCreateRetryableExceptionWithoutRetryAfterAndWithCause

Fixes OpenFeign#2458
rojae added a commit to rojae/feign that referenced this issue Apr 27, 2025
@rojae
Add constructor to RetryableException for cases without retryAfter
752db82 
Added two new constructors to RetryableException:
- A constructor without retryAfter and without cause
- A constructor without retryAfter but with cause

Standardized JavaDoc across all RetryableException constructors

These new constructors remove overload ambiguity when retryAfter is not needed.

Added a unit test to verify the new constructors.
- canCreateRetryableExceptionWithoutRetryAfter
- canCreateRetryableExceptionWithoutRetryAfterAndWithCause

Fixes OpenFeign#2458
rojae added a commit to rojae/feign that referenced this issue May 7, 2025
@rojae
Add. two new constructors to RetryableException:
5423711 
- A constructor without retryAfter and without cause
- A constructor without retryAfter but with cause

Standardized JavaDoc across all RetryableException constructors

These new constructors remove overload ambiguity when retryAfter is not needed.

Add. a unit test to verify the new constructors.
- canCreateRetryableExceptionWithoutRetryAfter
- canCreateRetryableExceptionWithoutRetryAfterAndWithCause

Fixes OpenFeign#2458
rojae added a commit to rojae/feign that referenced this issue May 7, 2025
@rojae
Add. two new constructors to RetryableException:
9c3bedf 
- A constructor without retryAfter and without cause
- A constructor without retryAfter but with cause

Standardized JavaDoc across all RetryableException constructors

These new constructors remove overload ambiguity when retryAfter is not needed.

Add. a unit test to verify the new constructors.
- canCreateRetryableExceptionWithoutRetryAfter
- canCreateRetryableExceptionWithoutRetryAfterAndWithCause

Fixes OpenFeign#2458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Issues that are good for first time contributors to tackle help wanted Issues we need help with tackling proposal Proposed Specification or API change
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add constructor to RetryableException for cases without retryAfter rojae/feign
2 participants
@kdavisk6 @heli-os