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

Clarify javadoc for ContentNegotiationConfigurer's ignoreAcceptHeader [SPR-13642] #18219

Closed
spring-issuemaster opened this Issue Nov 4, 2015 · 6 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Nov 4, 2015

Manuel Jordan opened SPR-13642 and commented

Hello

For the following classes:

Both have practically the same introduction or explanation, where I can see for example:

favorPathExtension PathExtensionContentNegotiationStrategy Yes

If I do click in favorPathExtension for each class I can read for each method description that the default value is Yes.
Same appreciation for favorParameter

favorParameter ParameterContentNegotiationStrategy -

Where here is false by default.

Until here, the introduction and method description match well. They are the same.

I have checked each row of the table (5 items)

Here two observations:

One:

defaultContentTypeStrategy ContentNegotiationStrategy -

If I do click in defaultContentTypeStrategy (for both classes) the setDefaultContentTypeStrategy does not indicate the default value.

Two: (here the reason of this post)

ignoreAcceptHeader HeaderContentNegotiationStrategy Yes

Theoretically the method description (for both classes) should be yes by default, but really says

By default this value is set to false.

How you can see it says false. Not yes how is expected.

I did not check the source code through GitHub, to see really what is the default value, but here there is no a match about the table against the method description. Here the error. So what is really the default value?.

Thanks.


Affects: 4.2.2

Referenced from: commits ac975df

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 5, 2015

Manuel Jordan commented

The error is in the javadoc for both classes only in the introduction:

According with the source code through GitHub

For ContentNegotiationConfigurer.java it has:

private final ContentNegotiationManagerFactoryBean factory =
			new ContentNegotiationManagerFactoryBean();

…

public ContentNegotiationConfigurer ignoreAcceptHeader(boolean ignoreAcceptHeader) {
	this.factory.setIgnoreAcceptHeader(ignoreAcceptHeader);
	return this;
}

Where ContentNegotiationManagerFactoryBean has:

private boolean ignoreAcceptHeader = false;

…


public void setIgnoreAcceptHeader(boolean ignoreAcceptHeader) {
	this.ignoreAcceptHeader = ignoreAcceptHeader;
}

Therefore:

  • Javadoc for the methods are ok, they say false.
  • Javadoc for the introduction are wrong, they say true. Must be false
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 6, 2015

Rossen Stoyanchev commented

I think the confusion comes from the fact the HTML table in class-level Javadoc is trying to indicate which strategies are on or off by default. The property for the "header strategy" however is negated ignoreAcceptHeader where false means Yes the Accept header is used. I'll improve the class level doc to eliminate confusion. Thanks!

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 6, 2015

Rossen Stoyanchev commented

The above mentioned Javadoc was introduced as part of a major update in 4.2.2 see commit. Hence the fix applies to 4.2.3 only.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 6, 2015

Manuel Jordan commented

The property for the "header strategy" however is negated ignoreAcceptHeader where false means Yes the Accept header is used

Yes, that's the problem.

I'll improve the class level doc to eliminate confusion. Thanks!

Thanks!

The above mentioned Javadoc was introduced as part of a major update in 4.2.2 see commit. Hence the fix applies to 4.2.3 only.

Sorry, I don't know what is the explicit part you have added to resolve this doubt. I remain with this kind of confusion yet. I suggest friendly and politely add a simple note below of the table and explain the trick about the context for the ignoreAcceptHeader.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 6, 2015

Manuel Jordan commented

Thanks I've checked…

If in the source code

private boolean ignoreAcceptHeader = false;

where is false

why in the updated javadoc appears:

ignoreAcceptHeader(boolean) Header strategy On

I think should be Off, but I see now your point about why is On. I think you should add your note about

The property for the "header strategy" however is negated ignoreAcceptHeader where false means Yes the Accept header is used

it to avoid any possible confusion.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment