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

Modify SetHeader method in the Context to enable the overriding of existing values. #839

Merged
merged 4 commits into from
Jul 7, 2023

Conversation

claudiamurialdo
Copy link
Collaborator

Issue:103629
Starting from #823, headers are set at the beginning of the execution. Therefore, in order for the changes made with the "SetHeader" method to take effect, overriding is necessary.

@claudiamurialdo claudiamurialdo requested a review from ggallotti July 5, 2023 13:38
@genexusbot
Copy link
Collaborator

Cherry pick to beta success

@claudiamurialdo claudiamurialdo temporarily deployed to external-storage-tests July 5, 2023 13:38 — with GitHub Actions Inactive
@genexusbot
Copy link
Collaborator

Cherry pick to beta success

@claudiamurialdo claudiamurialdo temporarily deployed to external-storage-tests July 6, 2023 11:30 — with GitHub Actions Inactive
@ggallotti
Copy link
Member

ggallotti commented Jul 6, 2023

@cmurialdo : This change is ok!

However, I think that the underlying implementation is loosing precision about the Cache-Control Header.

For example, in LigaMX, in order Http Rest Services to be Cacheable by CDN with (stale-while-revalidate) for getting maximum perfornace, I set this:

&HttpResponse.AddHeader(
	!"Cache-Control", 
	Format(!"public, s-maxage=%1, max-age=%1, stale-while-revalidate=15, stale-if-error=%2", 
	&MaxAge.ToString().Trim(), 
	&ErrorSeconds.ToString().Trim())
)

I wonder if this is supported by GeneXus .Net Generator.

This Cache Header is extremly important, as tells the CDN to go only on time to fetch the resource to the origin server, while retuning data from the Cache. It gives max scalability in an app.

Maybe we should use CacheControlHeaderValue.Parse(&CacheControlHeader) instead? Ant then fallback to the original implementation if the Header cannot be parsed.

(Maybe this is should be another Issue)

@genexusbot
Copy link
Collaborator

Cherry pick to beta success

@claudiamurialdo claudiamurialdo temporarily deployed to external-storage-tests July 6, 2023 14:59 — with GitHub Actions Inactive
@claudiamurialdo
Copy link
Collaborator Author

@cmurialdo : This change is ok!

However, I think that the underlying implementation is loosing precision about the Cache-Control Header.

For example, in LigaMX, in order Http Rest Services to be Cacheable by CDN with (stale-while-revalidate) for getting maximum perfornace, I set this:

&HttpResponse.AddHeader(
	!"Cache-Control", 
	Format(!"public, s-maxage=%1, max-age=%1, stale-while-revalidate=15, stale-if-error=%2", 
	&MaxAge.ToString().Trim(), 
	&ErrorSeconds.ToString().Trim())
)

I wonder if this is supported by GeneXus .Net Generator.

This Cache Header is extremly important, as tells the CDN to go only on time to fetch the resource to the origin server, while retuning data from the Cache. It gives max scalability in an app.

Maybe we should use CacheControlHeaderValue.Parse(&CacheControlHeader) instead? Ant then fallback to the original implementation if the Header cannot be parsed.

(Maybe this is should be another Issue)

@cmurialdo : This change is ok!

However, I think that the underlying implementation is loosing precision about the Cache-Control Header.

For example, in LigaMX, in order Http Rest Services to be Cacheable by CDN with (stale-while-revalidate) for getting maximum perfornace, I set this:

&HttpResponse.AddHeader(
	!"Cache-Control", 
	Format(!"public, s-maxage=%1, max-age=%1, stale-while-revalidate=15, stale-if-error=%2", 
	&MaxAge.ToString().Trim(), 
	&ErrorSeconds.ToString().Trim())
)

I wonder if this is supported by GeneXus .Net Generator.

This Cache Header is extremly important, as tells the CDN to go only on time to fetch the resource to the origin server, while retuning data from the Cache. It gives max scalability in an app.

Maybe we should use CacheControlHeaderValue.Parse(&CacheControlHeader) instead? Ant then fallback to the original implementation if the Header cannot be parsed.

(Maybe this is should be another Issue)

You are right. That case wasn't working. I changed the code as you suggested.

ggallotti
ggallotti previously approved these changes Jul 6, 2023
@claudiamurialdo claudiamurialdo temporarily deployed to external-storage-tests July 6, 2023 16:21 — with GitHub Actions Inactive
@genexusbot
Copy link
Collaborator

Cherry pick to beta success

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

Successfully merging this pull request may close these issues.

3 participants