-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix(providers): Sanitize nuts properties from configuration #221
fix(providers): Sanitize nuts properties from configuration #221
Conversation
2644c8d
to
f60c05d
Compare
426d87e
to
abeadcb
Compare
536c4cd
to
c37739f
Compare
@mattvb91 can you try with the |
@darkweak I can now see the keys succesfully purged from EDIT: also the |
What's your configuration file ? What are the sent requests ? That's really weird, the E2E tests are updated to ensure everything works as expected. |
So the order is the following. Restart caddy and have an empty list on both
This request goes to a
Both the API and frontend responses have been tagged with the
Now the API still has a
While the keys list is empty
And looking at it it may be caused by the Config: "cache":{
"api":{
"basepath":"/__cache",
"souin":{
"basepath":"/souin",
"enable":true
}
},
"cache_keys":{
"/_next\\/static/":{
"disable_host":true
}
},
"cdn":{
"dynamic":true,
"strategy":"hard"
},
"distributed":true,
"log_level":"debug",
"nuts":{
"path":"/tmp/nuts-souin"
},
"stale":"0s",
"ttl":"3600s"
} |
@mattvb91 I'm not able to reproduce so I'll try to fix that blindly. Can you try with the |
No luck with that, still gets stuck in the list:
and it doesnt get added back into the surrogate key list either at that point because im assuming the cache entry doesnt hold the Is there any way to disable the |
In my tests I didn't set a cache key with a comma + space, that's why it works on my side, my bad |
This commit should fix this bug.
No, because if the vary is not part of the cache key, you'll probably serve a wrong response that depend on the headers (e.g. language) |
Sorry deleted my last comment from 1 minute ago, I had accidently disabled This looks like it is working correctly on initial glance!!! Clearing both my frontend & API responses now! This is great. |
Is it really working ? 😅 |
Yup at least with requests that have 1 surrogate key per page. I will test tomorow with list pages that will have multiple surrogate keys to check that it flushes list pages too but yea this definitely works nicely for individual pages now great stuff 👍 |
So it looks like its 90% there. If I run my whole test suite including list pages that need to get flushed it fails when there is a large number of surrogate keys in the list. However if I restart caddy to clear the cache and run the individual test that fails which results in only a small number of surrogate keys then the test passes. So at the moment I cant even give a concrete example cause I need to dig a bit more to find out exactly whats happening but my initial suspicion is that once there is a large number of entries then theres some kind of corruption going on in the surrogate key list / cache entries and it isn't able to cleanly flush the specified resource. |
@darkweak not sure if this is related just looking into if I can get a certain order at which it fails but why do I get different header responses when using curl vs what the browser receives?
Same request on chrome dev tools:
Is that because it adds keys depending on request headers coming in from the browser? |
The browser send a curl like |
Thanks! Im still a bit lost as to why im getting different results I feel like it should give me back the same cache entry I must be missing something obvious?
Browser:
same etag but the content being served in both instances is actually old and hasnt been cleared. EDIT: under
However on
So it looks like 1 is getting lost in the cache and no longer tracked over surrogates |
8c05948
to
e445378
Compare
e445378
to
89ddf86
Compare
Fix #215 (comment)