Skip to content

Add StaticHeadersWriter.setIgnoreIfContainsHeader(boolean) #6478

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

Closed
rwinch opened this issue Jan 24, 2019 · 9 comments
Closed

Add StaticHeadersWriter.setIgnoreIfContainsHeader(boolean) #6478

rwinch opened this issue Jan 24, 2019 · 9 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement

Comments

@rwinch
Copy link
Member

rwinch commented Jan 24, 2019

Summary

We should add a property to StaticHeadersWriter that indicates it should skip adding a header if the header already exists.

@rwinch rwinch added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement status: ideal-for-contribution An issue that we actively are looking for someone to help us with labels Jan 24, 2019
@rwinch rwinch added this to the 5.2.x milestone Jan 24, 2019
@ankurpathak
Copy link
Contributor

ankurpathak commented Jan 25, 2019

@rwinch I would like to take this one.

@ankurpathak
Copy link
Contributor

ankurpathak commented Jan 25, 2019

@rwinch With this issue:
#6454
and corresponding pull request:
#6456
now be will be aready checking for header already exists
before writing any header in any HeaderWriter.
So does we need this Issue?

@mickaeltr
Copy link

mickaeltr commented Jan 25, 2019

Hi @ankurpathak,

I think the changes you've made in #6456 forStaticHeadersWriter, won't be enough for my use case (described at #6476):

if (!response.containsHeader(header.getName())) {
    response.setHeader(header.getName(), value);
} else {
    response.addHeader(header.getName(), value);
}

I would like the static header to be skipped if it already exists in the response. But with this code, it would be added, not skipped.

Thanks for helping.

@ankurpathak
Copy link
Contributor

ankurpathak commented Jan 25, 2019

@mickaeltr We can modify method something like this to acheive:


		for (Header header : headers) {
			for (String value : header.getValues()) {
				if(!hasHeader(response, header.getName(), value)){
					response.addHeader(header.getName(), value);
				}
			}
		}
	

	private boolean hasHeader(HttpServletResponse response, String headerName, String headerValue) {
		return response.containsHeader(headerName) && response.getHeaders(headerName).contains(headerValue);
	}

Changed in #6456 as well.

@mickaeltr
Copy link

mickaeltr commented Jan 25, 2019

@ankurpathak In my case, I think that still would not work

I would like my static Cache-Control to be: max-age=0, no-cache, must-revalidate

I would like to override it, for example with: max-age=3600

With your implementation, I understand that I would get max-age=3600, no-cache, must-revalidate instead of only max-age=3600.

So I am more in favor of an ignoreIfContainsHeader flag, as suggested by @rwinch. Besides that would avoid breaking backward compatibility.

@ankurpathak
Copy link
Contributor

ankurpathak commented Jan 25, 2019

@mickaeltr This will cover your case I think.

                for (Header header : headers) {
			if (!response.containsHeader(header.getName())) {
				for (String value : header.getValues()) {
					response.addHeader(header.getName(), value);
				}
			}
		}

Changed in #6456 as well.

@mickaeltr
Copy link

Yes that would work for me. 👍
However that would break backward compatibility… I am not sure what is the policy about that.

@ankurpathak
Copy link
Contributor

Yes that would work for me.
However that would break backward compatibility… I am not sure what is the policy about that.

Issue for which I have submitted PR yet to be reviewed. The issue itself is about checking header existence before writing them. I will be able to comment on backward compatabilty only after that. But in current state its not breaking any existing tests.

@rwinch rwinch added status: duplicate A duplicate of another issue and removed status: ideal-for-contribution An issue that we actively are looking for someone to help us with labels Jan 25, 2019
@rwinch rwinch removed this from the 5.2.x milestone Jan 25, 2019
@rwinch rwinch self-assigned this Jan 25, 2019
@rwinch
Copy link
Member Author

rwinch commented Jan 25, 2019

@ankurpathak Thanks for pointing that out. You are right this is a duplicate.

@mickaeltr You are right to worry about passivity. We will move that discussion to the existing tickets though

@rwinch rwinch closed this as completed Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants