-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
[rest] Add caching for static resources #3335
Conversation
This implements a new optional `cacheable` parameter for these REST endpoints: - `/rest/items` - `/rest/things` - `/rest/rules` When this parameter is set, a flat list of all elements excluding non-cacheable fields (e.g. "state", "transformedState", "stateDescription", "commandDescription" for items, "statusInfo", "firmwareStatus", "properties" for things, "status" for rules) will be retrieved along with a `Last-Modified` HTTP response header. When unknown, the Last-Modified header will be set to the date of the request. Also only when this parameter is set, and a `If-Modified-Since` header is found in the request, that header will be compared to the last known modified date for the corresponding cacheable list. The last modified date will be reset when any change is made on the elements of the underlying registry. If the `If-Modified-Since` date is equal or more recent than the last modified date, then a 304 Not Modified response with no content will be served instead of the usual 200 OK, informing the client that its cache is still valid at the provided date. All other request parameters will be ignored except for "metadata" in the `/rest/items` endpoint. When a metadata selector is set, the resulting item list will be considered like a completely different resource, i.e. it will have its own last modified date. Regarding metadata, the approach to invalidating last modified dates is very conservative: when any metadata is changed, all cacheable lists of items will have their last modified date reset even if the change was in a metadata namespace that wasn't requested. This also implements the abovedescribed behavior for the `/rest/ui/components/{namespace}` endpoint, but no `cacheable` parameter is necessary. The last modified date is tracked by namespace. Signed-off-by: Yannick Schaus <github@schaus.net>
Examples (using HTTPie): The first request at 08:24:14 GMT responds with a flat list of items retrieved with the cacheable parameter. Note the Last-Modified response header:
Supposing the client has cached this list, a second request is made with the If-Modified-Since set to the Last-Modified of the previous request (could also be the current date):
Of course, requests without the header still return the list (with or without Last-Modified depending on the cacheable parameter):
If any item (or metadata) was changed, then the list will be sent again, with a new Last-Modified header:
(note: the last modified date is the date when the first request was made after the actual change, not the real date of the change; since it's only precise to the second in those headers, it's to mitigate race conditions if changes and requests are made within the same second). Requests for UI components are cacheable by default:
If used carefully with client-side caching by UIs and other clients, this has the potential of saving a ton of bandwidth. |
Signed-off-by: Yannick Schaus <github@schaus.net>
Depends on openhab/openhab-core#3335. Will fetch items, things and rules with the `cacheable` parameter set when appropriate. Closes openhab#1650. Signed-off-by: Yannick Schaus <github@schaus.net>
Depends on openhab/openhab-core#3335. Will fetch items, things and rules with the `cacheable` parameter set when appropriate. Closes openhab#1650. Signed-off-by: Yannick Schaus <github@schaus.net>
@QueryParam("tags") final @Nullable List<String> tags, | ||
@QueryParam("summary") @Parameter(description = "summary fields only") @Nullable Boolean summary) { | ||
@QueryParam("summary") @Parameter(description = "summary fields only") @Nullable Boolean summary, | ||
@DefaultValue("false") @QueryParam("cacheable") @Parameter(description = "provides a cacheable list and honors the If-Modified-Since header, all other parameters are ignored") boolean cacheable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's a good naming. It is not clear what is actually returned if cachable=true
is used. It would be better to add a parameter to return structural/static content only (i.e. without status/state). If that is set (which means, this is data that is likely not going to change), the If-Modified-Since
header should be processed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to add a parameter to return structural/static content only (i.e. without status/state)
That's basically that this parameter does, I'll admit it was not my first choice either, but after thinking long and hard this is the best I could come up with. It conveys that it will only output information that is ready to be cached (and nothing more) along with the headers to let the browsers do it.
I'm open to suggestions to improve the name 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about "suppressRuntimeData"? Or "limitToStaticData"?
Signed-off-by: Yannick Schaus <github@schaus.net>
Signed-off-by: Yannick Schaus <github@schaus.net>
I've switched OSes and I don't have a working Java development environment anymore, at least at the moment. I think the debate was mostly about the name of the parameter so it could be decided and the code could be updated. |
Okay, thanks for the status. Regarding the naming, something like |
@ghys I like the naming suggested by @florian-h05. Would you update this PR and rebase, so we can merge? |
DCO is failing, but other then this it seems finished to me, ping @J-N-K. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@ghys Can you rebase to solve the merge conflict and check the sign-off of you last commit? |
Signed-off-by: Yannick Schaus <github@schaus.net>
Yeah please bear with me, I'm using this PR and made the changes "in the blind" to check out the GitHub Web IDE (https://github.dev/openhab/openhab-core/pull/3335) as a contributor. As for the rebase this is also a good opportunity to test https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/, I've activated it on the webui repo for a while (it's under "General" now, they've designed the settings) so you can rebase yourself if needed, it's always nice to have the option instead of asking the contributor when merging stale PRs. |
-- Signed-off-by: Yannick Schaus <github@schaus.net>
...re.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/RuleResource.java
Outdated
Show resolved
Hide resolved
…nhab/core/automation/rest/internal/RuleResource.java Signed-off-by: J-N-K <github@klug.nrw>
...re.automation.rest/src/main/java/org/openhab/core/automation/rest/internal/RuleResource.java
Outdated
Show resolved
Hide resolved
…nhab/core/automation/rest/internal/RuleResource.java Signed-off-by: J-N-K <github@klug.nrw>
Closes #1650. Depends on openhab/openhab-core#3335. Will fetch items, things and rules with the `staticDataOnly` parameter set when appropriate. Also-by: Florian Hotze <florianh_dev@icloud.com> Signed-off-by: Yannick Schaus <github@schaus.net>
Fix openhab#3679 If not, calls of method limitToFields by PR openhab#3335 fails when the list of fields does not contain firmwareStatus. Signed-off-by: Laurent Garnier <lg.hc@free.fr>
* Closes openhab#3329. This implements a new optional `cacheable` parameter for these REST endpoints: - `/rest/items` - `/rest/things` - `/rest/rules` When this parameter is set, a flat list of all elements excluding non-cacheable fields (e.g. "state", "transformedState", "stateDescription", "commandDescription" for items, "statusInfo", "firmwareStatus", "properties" for things, "status" for rules) will be retrieved along with a `Last-Modified` HTTP response header. When unknown, the Last-Modified header will be set to the date of the request. Also only when this parameter is set, and a `If-Modified-Since` header is found in the request, that header will be compared to the last known modified date for the corresponding cacheable list. The last modified date will be reset when any change is made on the elements of the underlying registry. If the `If-Modified-Since` date is equal or more recent than the last modified date, then a 304 Not Modified response with no content will be served instead of the usual 200 OK, informing the client that its cache is still valid at the provided date. All other request parameters will be ignored except for "metadata" in the `/rest/items` endpoint. When a metadata selector is set, the resulting item list will be considered like a completely different resource, i.e. it will have its own last modified date. Regarding metadata, the approach to invalidating last modified dates is very conservative: when any metadata is changed, all cacheable lists of items will have their last modified date reset even if the change was in a metadata namespace that wasn't requested. This also implements the abovedescribed behavior for the `/rest/ui/components/{namespace}` endpoint, but no `cacheable` parameter is necessary. The last modified date is tracked by namespace. Signed-off-by: Yannick Schaus <github@schaus.net> GitOrigin-RevId: 6e83d3f
Fix openhab#3679 If not, calls of method limitToFields by PR openhab#3335 fails when the list of fields does not contain firmwareStatus. Signed-off-by: Laurent Garnier <lg.hc@free.fr> GitOrigin-RevId: e9719eb
CacheControl cc = new CacheControl(); | ||
cc.setMustRevalidate(true); | ||
cc.setPrivate(true); | ||
thingStream = dtoMapper.limitToFields(thingStream, "UID,label,bridgeUID,thingTypeUID,location,editable"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghys @J-N-K Is is possible that channels change without a notification to the registry?
If not, we should add channels here to fix openhab/openhab-webui#1996 - the alternative would be to not use the cached list there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@J-N-K Sorry for pinging again, but I think you can answer my question above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it might be possible, but I would consider it a bug in the binding. You might be able to edit the thing, but if you do it right (using ThingBuilder thingBuilder = editThing();
and updateThing(thingBuilder.build);
) you'll get a notification about an updated thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ghys @J-N-K Is is possible that channels change without a notification to the registry?
If not, we should add channels here to fix openhab/openhab-webui#1996 - the alternative would be to not use the cached list there.
it's IMO way better to just change the channeltype-picker in the UI to not use cached lists. There is no need to add channels (a huge chunk of the response payload, some things have dozens if not hundreds of channels), they're not used apart from that picker and there is only one instance of them in the entire UI.
Closes #3329.
This implements a new optional
cacheable
parameter for these REST endpoints:/rest/items
/rest/things
/rest/rules
When this parameter is set, a flat list of all elements excluding non-cacheable fields (e.g. "state", "transformedState", "stateDescription", "commandDescription" for items, "statusInfo", "firmwareStatus", "properties" for things, "status" for rules) will be retrieved along with a
Last-Modified
HTTP response header. When unknown, the Last-Modified header will be set to the date of the request.Also only when this parameter is set, and a
If-Modified-Since
header is found in the request, that header will be compared to the last known modified date for the corresponding cacheable list. The last modified date will be reset when any change is made on the elements of the underlying registry. If theIf-Modified-Since
date is equal or more recent than the last modified date, then a 304 Not Modified response with no content will be served instead of the usual 200 OK, informing the client that its cache is still valid at the provided date.All other request parameters will be ignored except for "metadata" in the
/rest/items
endpoint. When a metadata selector is set, the resulting item list will be considered like a completely different resource, i.e. it will have its own last modified date. Regarding metadata, the approach to invalidating last modified dates is very conservative: when any metadata is changed, all cacheable lists of items will have their last modified date reset even if the change was in a metadata namespace that wasn't requested.This also implements the abovedescribed behavior for the
/rest/ui/components/{namespace}
endpoint, but nocacheable
parameter is necessary. The last modified date is tracked by namespace.Signed-off-by: Yannick Schaus github@schaus.net