From 669430666055d2f14cd17575e87519f312775baf Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Mon, 27 Feb 2023 13:45:15 +0100 Subject: [PATCH] Use If-Unmodified-Since istead of If-Match If-Match relies on ETag, which depends on the Content-Encoding, which is more fragile given we updated the etag on save, while normal files are served by nginx. So this may occurs false mismatch. --- umap/static/umap/js/umap.layer.js | 6 ++--- umap/tests/test_datalayer_views.py | 21 +++++++----------- umap/views.py | 35 +++++++++++++----------------- 3 files changed, 26 insertions(+), 36 deletions(-) diff --git a/umap/static/umap/js/umap.layer.js b/umap/static/umap/js/umap.layer.js index 2aa8929e..4a77571c 100644 --- a/umap/static/umap/js/umap.layer.js +++ b/umap/static/umap/js/umap.layer.js @@ -274,7 +274,7 @@ L.U.DataLayer = L.Evented.extend({ if (!this.umap_id) return; this.map.get(this._dataUrl(), { callback: function (geojson, response) { - this._etag = response.getResponseHeader('ETag'); + this._last_modified = response.getResponseHeader('Last-Modified'); this.fromUmapGeoJSON(geojson); this.backupOptions(); this.fire('loaded'); @@ -1017,7 +1017,7 @@ L.U.DataLayer = L.Evented.extend({ data: formData, callback: function (data, response) { this._geojson = geojson; - this._etag = response.getResponseHeader('ETag'); + this._last_modified = response.getResponseHeader('Last-Modified'); this.setUmapId(data.id); this.updateOptions(data); this.backupOptions(); @@ -1028,7 +1028,7 @@ L.U.DataLayer = L.Evented.extend({ this.map.continueSaving(); }, context: this, - headers: {'If-Match': this._etag || ''} + headers: this._last_modified ? {'If-Unmodified-Since': this._last_modified} : {} }); }, diff --git a/umap/tests/test_datalayer_views.py b/umap/tests/test_datalayer_views.py index 01fd5c64..ef00b822 100644 --- a/umap/tests/test_datalayer_views.py +++ b/umap/tests/test_datalayer_views.py @@ -24,11 +24,8 @@ def post_data(): def test_get(client, settings, datalayer): url = reverse('datalayer_view', args=(datalayer.pk, )) response = client.get(url) - if getattr(settings, 'UMAP_XSENDFILE_HEADER', None): - assert response['ETag'] is not None assert response['Last-Modified'] is not None assert response['Cache-Control'] is not None - assert response['Vary'] == 'Accept-Encoding' assert 'Content-Encoding' not in response j = json.loads(response.content.decode()) assert '_umap_options' in j @@ -91,46 +88,44 @@ def test_should_not_be_possible_to_delete_with_wrong_map_id_in_url(client, datal def test_get_gzipped(client, datalayer, settings): url = reverse('datalayer_view', args=(datalayer.pk, )) response = client.get(url, HTTP_ACCEPT_ENCODING='gzip') - if getattr(settings, 'UMAP_XSENDFILE_HEADER', None): - assert response['ETag'] is not None assert response['Last-Modified'] is not None assert response['Cache-Control'] is not None assert response['Content-Encoding'] == 'gzip' -def test_optimistic_concurrency_control_with_good_etag(client, datalayer, map, post_data): # noqa - # Get Etag +def test_optimistic_concurrency_control_with_good_last_modified(client, datalayer, map, post_data): # noqa + # Get Last-Modified url = reverse('datalayer_view', args=(datalayer.pk, )) response = client.get(url) - etag = response['ETag'] + last_modified = response['Last-Modified'] url = reverse('datalayer_update', args=(map.pk, datalayer.pk)) client.login(username=map.owner.username, password="123123") name = 'new name' post_data['name'] = 'new name' - response = client.post(url, post_data, follow=True, HTTP_IF_MATCH=etag) + response = client.post(url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=last_modified) assert response.status_code == 200 modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) assert modified_datalayer.name == name -def test_optimistic_concurrency_control_with_bad_etag(client, datalayer, map, post_data): # noqa +def test_optimistic_concurrency_control_with_bad_last_modified(client, datalayer, map, post_data): # noqa url = reverse('datalayer_update', args=(map.pk, datalayer.pk)) client.login(username=map.owner.username, password='123123') name = 'new name' post_data['name'] = name - response = client.post(url, post_data, follow=True, HTTP_IF_MATCH='xxx') + response = client.post(url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE='xxx') assert response.status_code == 412 modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) assert modified_datalayer.name != name -def test_optimistic_concurrency_control_with_empty_etag(client, datalayer, map, post_data): # noqa +def test_optimistic_concurrency_control_with_empty_last_modified(client, datalayer, map, post_data): # noqa url = reverse('datalayer_update', args=(map.pk, datalayer.pk)) client.login(username=map.owner.username, password='123123') name = 'new name' post_data['name'] = name - response = client.post(url, post_data, follow=True, HTTP_IF_MATCH=None) + response = client.post(url, post_data, follow=True, HTTP_IF_UNMODIFIED_SINCE=None) assert response.status_code == 200 modified_datalayer = DataLayer.objects.get(pk=datalayer.pk) assert modified_datalayer.name == name diff --git a/umap/views.py b/umap/views.py index 709b3be9..1f9cdcac 100644 --- a/umap/views.py +++ b/umap/views.py @@ -693,12 +693,10 @@ class GZipMixin(object): def path(self): return self.object.geojson.path - def etag(self): - # Align ETag with Nginx one, because when using X-Send-File, If-None-Match - # and If-Modified-Since are handled by Nginx. - # https://github.com/nginx/nginx/blob/4ace957c4e08bcbf9ef5e9f83b8e43458bead77f/src/http/ngx_http_core_module.c#L1675-L1709 - statobj = os.stat(self.path) - return '"%x-%x"' % (int(statobj.st_mtime), statobj.st_size) + @property + def last_modified(self): + stat = os.stat(self.path) + return http_date(stat.st_mtime) class DataLayerView(GZipMixin, BaseDetailView): @@ -727,8 +725,7 @@ class DataLayerView(GZipMixin, BaseDetailView): with open(path, "rb") as f: # Should not be used in production! response = HttpResponse(f.read(), content_type="application/geo+json") - response["Last-Modified"] = http_date(statobj.st_mtime) - response["ETag"] = self.etag() + response["Last-Modified"] = self.last_modified response["Content-Length"] = statobj.st_size response["Vary"] = "Accept-Encoding" if accepts_gzip and settings.UMAP_GZIP: @@ -737,7 +734,6 @@ class DataLayerView(GZipMixin, BaseDetailView): class DataLayerVersion(DataLayerView): - @property def path(self): return "{root}/{path}".format( @@ -755,7 +751,7 @@ class DataLayerCreate(FormLessEditMixin, GZipMixin, CreateView): self.object = form.save() # Simple response with only metadatas (including new id) response = simple_json_response(**self.object.metadata) - response["ETag"] = self.etag() + response["Last-Modified"] = self.last_modified return response @@ -768,24 +764,23 @@ class DataLayerUpdate(FormLessEditMixin, GZipMixin, UpdateView): # Simple response with only metadatas (client should not reload all data # on save) response = simple_json_response(**self.object.metadata) - response["ETag"] = self.etag() + response["Last-Modified"] = self.last_modified return response - def if_match(self): + def is_unmodified(self): """Optimistic concurrency control.""" - match = True - if_match = self.request.META.get("HTTP_IF_MATCH") - if if_match: - etag = self.etag() - if etag != if_match: - match = False - return match + modified = True + if_unmodified = self.request.META.get("HTTP_IF_UNMODIFIED_SINCE") + if if_unmodified: + if self.last_modified != if_unmodified: + modified = False + return modified def post(self, request, *args, **kwargs): self.object = self.get_object() if self.object.map != self.kwargs["map_inst"]: return HttpResponseForbidden() - if not self.if_match(): + if not self.is_unmodified(): return HttpResponse(status=412) return super(DataLayerUpdate, self).post(request, *args, **kwargs)