Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-13642

Clarify javadoc for ContentNegotiationConfigurer's ignoreAcceptHeader

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Complete
    • Affects Version/s: 4.2.2
    • Fix Version/s: 4.2.3
    • Component/s: [Documentation]
    • Labels:
      None
    • Last commented by a User:
      true

      Description

      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.

        Activity

        Hide
        dr_pompeii Manuel Jordan added a comment -

        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
        Show
        dr_pompeii Manuel Jordan added a comment - 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
        Hide
        rstoya05-aop Rossen Stoyanchev added a comment -

        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 *ignore*AcceptHeader where false means Yes the Accept header is used. I'll improve the class level doc to eliminate confusion. Thanks!

        Show
        rstoya05-aop Rossen Stoyanchev added a comment - 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 * ignore *AcceptHeader where false means Yes the Accept header is used. I'll improve the class level doc to eliminate confusion. Thanks!
        Hide
        rstoya05-aop Rossen Stoyanchev added a comment -

        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.

        Show
        rstoya05-aop Rossen Stoyanchev added a comment - 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.
        Hide
        dr_pompeii Manuel Jordan added a comment -

        The property for the "header strategy" however is negated *ignore*AcceptHeader 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.

        Show
        dr_pompeii Manuel Jordan added a comment - The property for the "header strategy" however is negated *ignore*AcceptHeader 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.
        Show
        rstoya05-aop Rossen Stoyanchev added a comment - - edited Check the table in the class-level Javadoc: http://docs.spring.io/spring-framework/docs/4.2.3.BUILD-SNAPSHOT/javadoc-api/org/springframework/web/servlet/config/annotation/ContentNegotiationConfigurer.html .
        Hide
        dr_pompeii Manuel Jordan added a comment - - edited

        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 *ignore*AcceptHeader where false means Yes the Accept header is used

        it to avoid any possible confusion.

        Thanks

        Show
        dr_pompeii Manuel Jordan added a comment - - edited 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 *ignore*AcceptHeader where false means Yes the Accept header is used it to avoid any possible confusion. Thanks

          People

          • Assignee:
            rstoya05-aop Rossen Stoyanchev
            Reporter:
            dr_pompeii Manuel Jordan
            Last updater:
            Stéphane Nicoll
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:
              Days since last comment:
              2 years, 16 weeks, 1 day ago