Skip to content
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

GH-2602: Fix x-delay header to Long #2603

Closed
wants to merge 7 commits into from
Closed

GH-2602: Fix x-delay header to Long #2603

wants to merge 7 commits into from

Conversation

raylax
Copy link
Contributor

@raylax raylax commented Jan 17, 2024

artembilan
artembilan requested changes Jan 17, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thank you for fast turn around!

Please, consider to add you name to the @author list of all the affected classes.

spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Show resolved Hide resolved
spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Show resolved Hide resolved
spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Outdated Show resolved Hide resolved
spring-amqp/src/main/java/org/springframework/amqp/support/SimpleAmqpHeaderMapper.java Outdated Show resolved Hide resolved
spring-amqp/src/test/java/org/springframework/amqp/support/SimpleAmqpHeaderMapperTests.java Outdated
@@ -95,6 +95,26 @@ public void fromHeaders() {
assertThat(amqpProperties.getDelay()).isEqualTo(Integer.valueOf(1234));
Copy link
Member

Choose a reason for hiding this comment

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

Please, consider to not use a deprecated API any more.

Copy link
Member

Choose a reason for hiding this comment

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

I don't believe in supporting deprecated API.
Plus after fixing it with a delegating to a new API, there is no need in double coverage.

spring-amqp/src/test/java/org/springframework/amqp/support/SimpleAmqpHeaderMapperTests.java Outdated Show resolved Hide resolved
@raylax
Copy link
Contributor Author

raylax commented Jan 18, 2024

Thanks for the review.

Is it okay if I change it like this?

@raylax raylax requested a review from artembilan January 18, 2024 14:27
artembilan
artembilan requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

You have not fixed all the usage of the deprecated API yet:

 /home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/core/RabbitAdminIntegrationTests.java:400: warning: [removal] setDelay(Integer) in MessageProperties has been deprecated and marked for removal
			message.getMessageProperties().setDelay(1000);
			                              ^
/home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/core/RabbitAdminIntegrationTests.java:404: warning: [removal] setDelay(Integer) in MessageProperties has been deprecated and marked for removal
		properties.setDelay(500);
		          ^
/home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/core/RabbitAdminIntegrationTests.java:410: warning: [removal] getReceivedDelay() in MessageProperties has been deprecated and marked for removal
		assertThat(received.getMessageProperties().getReceivedDelay()).isEqualTo(Integer.valueOf(500));
		                                          ^
/home/runner/work/spring-amqp/spring-amqp/spring-rabbit/src/test/java/org/springframework/amqp/rabbit/core/RabbitAdminIntegrationTests.java:413: warning: [removal] getReceivedDelay() in MessageProperties has been deprecated and marked for removal
		assertThat(received.getMessageProperties().getReceivedDelay()).isEqualTo(Integer.valueOf(1000));

spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Outdated Show resolved Hide resolved
@raylax raylax requested a review from artembilan January 18, 2024 15:46
artembilan
artembilan requested changes Jan 18, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

I see more deprecated API usage:

/home/runner/work/spring-amqp/spring-amqp/spring-amqp/src/test/java/org/springframework/amqp/support/SimpleAmqpHeaderMapperTests.java:97: warning: [removal] getDelay() in MessageProperties has been deprecated and marked for removal
		assertThat(amqpProperties.getDelay()).isEqualTo(Integer.valueOf(1234));
		                         ^
> Task :spring-amqp:compileTestJava
/home/runner/work/spring-amqp/spring-amqp/spring-amqp/src/test/java/org/springframework/amqp/support/SimpleAmqpHeaderMapperTests.java:157: warning: [removal] setReceivedDelay(Integer) in MessageProperties has been deprecated and marked for removal
		amqpProperties.setReceivedDelay(1234);
		              ^
/home/runner/work/spring-amqp/spring-amqp/spring-amqp/src/test/java/org/springframework/amqp/core/MessagePropertiesTests.java:58: warning: [removal] setDelay(Integer) in MessageProperties has been deprecated and marked for removal
> Task :checkstyleNohttp
> Task :check
		properties.setDelay(delay);
		          ^
/home/runner/work/spring-amqp/spring-amqp/spring-amqp/src/test/java/org/springframework/amqp/core/MessagePropertiesTests.java:60: warning: [removal] setDelay(Integer) in MessageProperties has been deprecated and marked for removal
		properties.setDelay(null);

Please, consider to run ./gradlew check locally before pushing changes.

spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Outdated
@@ -66,6 +69,8 @@ public class MessageProperties implements Serializable {

public static final Integer DEFAULT_PRIORITY = 0;

public static final long X_DELAY_MAX = 0xffffffffL; // (2 ^ 32) - 1
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this has to be public.

Copy link
Member

Choose a reason for hiding this comment

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

Please, add Javadoc for this new public API then.
Including @since

@raylax
Copy link
Contributor Author

raylax commented Jan 19, 2024 •
edited
Loading

Remaining deprecated API usage is to ensure that the old setDelay method works.

X_DELAY_MAX is set to public for user pre-checking.
like this

if (delay > X_DELAY_MAX) {
	// do something
}
rabbitTemplate.convertAndSend(...)

artembilan
artembilan requested changes Jan 19, 2024
View reviewed changes
Copy link
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Thanks for the update, but I still insist in removing usage of a deprecated API.
Thanks

spring-amqp/src/main/java/org/springframework/amqp/core/MessageProperties.java Outdated
@@ -66,6 +69,8 @@ public class MessageProperties implements Serializable {

public static final Integer DEFAULT_PRIORITY = 0;

public static final long X_DELAY_MAX = 0xffffffffL; // (2 ^ 32) - 1
Copy link
Member

Choose a reason for hiding this comment

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

Please, add Javadoc for this new public API then.
Including @since

spring-amqp/src/test/java/org/springframework/amqp/support/SimpleAmqpHeaderMapperTests.java Outdated
@@ -95,6 +95,26 @@ public void fromHeaders() {
assertThat(amqpProperties.getDelay()).isEqualTo(Integer.valueOf(1234));
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe in supporting deprecated API.
Plus after fixing it with a delegating to a new API, there is no need in double coverage.

@raylax
Copy link
Contributor Author

raylax commented Jan 20, 2024

All problems have been fixed, and I have run the ./gradlew check.

Thanks for your help.

@artembilan
Copy link
Member

Merged as 966338e.

@raylax ,

thank you for contribution; looking forward for more!

@artembilan artembilan closed this Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@artembilan artembilan artembilan requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@raylax @artembilan