WIP pull request! Will add another commit to update the swagger.json
after the initial review. Notes taken during code writing below:
In methods for resources like /vocabularies, which return collections that do not have a timestamp, before the collection is retrieved are now doing the checks for git/config modified dates, and using them if request contains the right headers.
I used https://petstore.swagger.io/#/ with the swagger.json from https://raw.githubusercontent.com/NatLibFi/Skosmos/master/swagger.json, going from the top to bottom. So the comments below should reflect what I found while progressing through the methods.
The code to return the 304 header was added after validation methods. Except validation that required
performing a query (otherwise returning 304 after that would be pointless performance-wise I think).
Manual tests were performed while implementing the changes with the following commands:
# First request before the change, with response below (NB date is in the future!)
$ curl -I --header 'If-Modified-Since: 2020-04-29 07:39:48' http://localhost:8000/rest/v1/yso/
HTTP/1.1 200 OK
Date: Wed, 24 Apr 2019 08:59:09 GMT
Server: Apache/2.4.25 (Debian)
X-Powered-By: PHP/7.0.33
Access-Control-Allow-Origin: *
Vary: Accept
Content-Type: application/json; charset=utf-8
# Second request after implementing change
$ curl -I --header 'If-Modified-Since: 2020-04-29 07:39:48' http://localhost:8000/rest/v1/yso/
HTTP/1.0 304 Not Modified
Date: Wed, 24 Apr 2019 09:01:05 GMT
Server: Apache/2.4.25 (Debian)
Connection: close
# Last request where the date is in the past (meaning the resource was modified after that, so no 304 expected)
HTTP/1.1 200 OK
Date: Wed, 24 Apr 2019 09:01:08 GMT
Server: Apache/2.4.25 (Debian)
X-Powered-By: PHP/7.0.33
Access-Control-Allow-Origin: *
Last-Modified: 2019-04-24 07:39:48
Vary: Accept
Content-Type: application/json; charset=utf-8
First modification: move methods related to modified date to Controller
The first commit moves the methods related to modified dates to the base Controller class.
Second modification: per vocabulary setting to enable/disable modified date
In the concept method for WebController, we check if the Vocabulary has the setting skosmos:useModifiedDate
, but there is no global setting to enable / disable the modified date globally. I thought we could have a global one and also a per vocabulary setting... but that would make troubleshooting it harder... in the end I thought the easiest approach would be to simply make skosmos:modifiedDate
a global configuration. What do you think?
Third modification: method signature for getModifiedDate
getModifiedDate
was expecting a Concept. So now it allows this concept to be null. This way it can
still be used by methods with a Concept, but otherwise simply skip that value if it was not provided.
It was also expecting a Vocabulary in the method. But then I realized it was not necessary, as the
Concept contains already a getVocab() to retrieve the Concept's Vocabulary. It won't affect the existing
concept method in WebController, as the Concept is obtained in the first place by calling a method on the Vocabulary (IOW, the Concept->getVocab() will return the same as before).
Fourth modification: realized I was always doing:
$useModifiedDate = $this->model->getConfig()->getUseModifiedDate();
if ($useModifiedDate) {
$modifiedDate = $this->getModifiedDate(null);
// return if the controller sends the not modified header
if ($this->sendNotModifiedHeader($modifiedDate)) {
return;
}
}
So created the helper method notModified(Concept)
in Controller
, and now the same code is simplified to:
if ($this->notModified(null)) {
return;
}
Methods that were not modified
I think for some methods, it may not make sense to return not modified, for some content
that the client may not have cached:
- data: it looks strange to return 304 for a url that may start the download of a file... what do you think?
- search: I have never seen a search result returning 304 I think... what do you think?
Q: I realized that I was not using the Concept
in the REST methods related to concepts... but the reason is that the methods used by those REST methods are querying single string values (e.g. label), never returning a complete Concept. I could alter each query to return the dc:modified
too, change the method to return... an array? Then use the dc:modified
value. Is that the right direction?
enhancement may slip