Adding methods to add Cache-Control and ETags #452
Conversation
|
Looks reasonable so far. We should add a check for the header to at least one of the existing integration tests. |
| void test_state_has_ETag_and_Cache_Control() { | ||
| String matcher; | ||
| given() | ||
| .log().ifValidationFails(LogDetail.ALL,true) |
There was a problem hiding this comment.
Good test, ETag shows up, Cache-Control does not.
FYI had to merge with the apikey fix. one of the things I fixed there was preventing the proper retrieval of the datasource.
There was a problem hiding this comment.
Oh. I think I know whats going on. At least I hope I do. In my testing I saw that it adds it as cache-control all lowercase. according to SO http/2.0 says that the header field names must be converted to lower case.
https://stackoverflow.com/a/41169947/249623
There was a problem hiding this comment.
I still haven't been able to get my IDE to run things. I'm working on something else at the moment but its annoying.
There was a problem hiding this comment.
The test I ran printed the headers, the cache-control header wasn't present for any casing... which was surprising I didn't see anything wrong with the code you made.
What kind of errors is you're IDE giving you? Karl had some issues with Intellij and Ant due to a version mismatch. Might be something similar since we need to fix the build to allow a java11+ compiler to work even if we are still targetting 8.
There was a problem hiding this comment.
Also, It's reasonable to put those header expectations in the test_state_catalog. Seems a waste of resources to just make the request again.
Though an interesting test would be to make the request again right after and see if it's cached, but I'm not sure if the restassured client actually does that.
|
|
||
| private static void addCacheControl(@NotNull String path, long duration, TimeUnit timeUnit) { | ||
| if(timeUnit != null && duration > 0) { | ||
| staticInstance().after(path, ctx -> { |
There was a problem hiding this comment.
This is matching on the exact string passed in. in the case of states it's waiting for exactly /states/{state}
This worked: ... after(path.replaceAll("(.*)(\\{.*\\})","$1*"), ...
There was a problem hiding this comment.
States was maybe a bad controller to test this with b/c States doesn't have a getOne method. Having the replacement string in the path does work for matching requests that include the resourceId but it doesn't match the getAll style requests. I played around with the regex a bit but I couldnt' get it to work to match both types of url so I just add two after handlers.
There was a problem hiding this comment.
I would say it was a good controller as it exposed a problem.
There was a problem hiding this comment.
oh, it should be (.*)(\\{.*\\})? so that the path parameter is optional... though that wouldn't match if we include multiple path parameters so maybe a replaceAll with just the \\{.*\\} -> * would do the trick.
There was a problem hiding this comment.
I added a test where you can play around with the regex approach. I couldn't get it to work - from what I can see the after handler system doesn't use regex - they do their own custom matching and manipulation.
There was a problem hiding this comment.
the regex isn't used by the after handler, it just substitutes an "{text}" with with * so "/locations/{location-id}" becomes "/locations/*" as "/path/*" was the example shown.
Though I can except that that still isn't quite enough, the example was limited and some of our handler path can be a bit more complex.
…d out a method to do the same path manipulation as cdaCrud does for generating the paths. Added test for each type in OfficeControllerIT. Added a property that can be specified to override HQ office.
rma-rripken
changed the title
|
@MikeNeilson I think this is ready |
|
This relates to contract W912HQ22F0193 Task 8 Topic A. |
I haven't been able to test this extensively b/c my IDE is suddenly complaining about flogger.