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.

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

[RESTEASY-3324] Enable redirects using property. #3710

Merged
merged 1 commit into from Aug 2, 2023

Conversation

The-Huginn
Copy link
Contributor

@wildfly-ci
Copy link
Collaborator

Hello, The-Huginn. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@jamezp
Copy link
Contributor

jamezp commented Jul 18, 2023

/ok-to-test

Copy link
Contributor

@jamezp jamezp left a comment

Choose a reason for hiding this comment

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

This would also need to be added to the documentation in the RESTEasy_Client_Framework.xml in the docbook directory as well.

@@ -279,5 +281,7 @@ public enum HostnameVerificationPolicy {
*/
public abstract ResteasyClientBuilder setFollowRedirects(boolean followRedirects);

public abstract ResteasyClientBuilder setFollowRedirects();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed since there is a setter above.

Comment on lines 166 to 168

@Message(id = BASE + 195, value = "Unable to set follow redirects policy.")
String unableToSetFollowRedirects();
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the comment below we likely don't need this.

Comment on lines 672 to 685
ClientConfiguration config = new ClientConfiguration(getProviderFactory());
try {
if (config.hasProperty(ResteasyClientBuilder.PROPERTY_FOLLOW_REDIRECTS)) {
Object enabled = config.getProperty(ResteasyClientBuilder.PROPERTY_FOLLOW_REDIRECTS);
if (enabled instanceof Boolean) {
this.followRedirects = (Boolean) enabled;
} else if (enabled instanceof String) {
this.followRedirects = Boolean.parseBoolean((String) enabled);
}
}
} catch (Exception e) {
// catch possible exceptions (in this case we do not set follow-redirects at all)
LogMessages.LOGGER.warn(Messages.MESSAGES.unableToSetFollowRedirects(), e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't quite right. We need to remove this whole method and check the property in the build() method. In the build() method it should just look something like:

final Object localFollowRedirects = config.getProperty(PROPERTY_FOLLOW_REDIRECTS);
if (localFollowRedirects != null) {
    followRedirects = Boolean.parseBoolean(String.valueOf(localFollowRedirects));
}

@@ -50,6 +50,8 @@ public enum HostnameVerificationPolicy {
*/
public static final String PROPERTY_PROXY_SCHEME = "org.jboss.resteasy.jaxrs.client.proxy.scheme";

public static final String PROPERTY_FOLLOW_REDIRECTS = "org.jboss.resteasy.jaxrs.client.follow-redirects";
Copy link
Contributor

Choose a reason for hiding this comment

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

We have started to change the prefix we use. This should be something like dev.resteasy.client.follow.redirects.

@The-Huginn
Copy link
Contributor Author

Currently I assume, the property has higher priority. Is that expected or do I add additional logic to allow overriding property by setFollowRedirects()?

@jamezp
Copy link
Contributor

jamezp commented Jul 19, 2023

Currently I assume, the property has higher priority. Is that expected or do I add additional logic to allow overriding property by setFollowRedirects()?

It's a tough one, but yes we are saying the property wins. If a user did something like:

ResteasyClientBuilder().setFollowRedirects(false)
    .property("dev.resteasy.client.follow.redirect", "true");

The latter would win. However, I think that's okay as then the user doesn't need to use the ResteasyClientBuilder directly.

@jamezp jamezp merged commit 030c995 into resteasy:main Aug 2, 2023
16 checks passed
@jamezp jamezp mentioned this pull request Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants