Skip to content
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.

Sign up for GitHub

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

[#324] Having startAt and endAt on today caused former objects to be shown #325

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[#324] Having startAt and endAt on today caused former objects to be shown #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77056dc
:bug: [#324] Fixed retrieval of overlapping objects by filtering with…
alextreme Mar 23, 2023
767a91f
:green_heart: Pin CI-build on ubuntu-20.04
alextreme Mar 28, 2023
889ce02
:rocket: Ensuring docker-compose-quickstart runs
alextreme Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ env:
jobs:
tests:
name: Run the Django test suite
runs-on: ubuntu-latest
runs-on: ubuntu-20.04
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

It had to be in the base libraries and thanks to this, I could find the real issue: Ubuntu 20 uses GDAL 2.4 and Ubuntu 22 uses GDAL 3.0. This lead to this Stackoverflow question: https://stackoverflow.com/questions/62939518/geodjango-gdal-3-coordinates-order-changes

And thus the real issue is that we need to update to Django 3.1+ to get GDAL 3 support.

However, this update, together with the Docker base image update, should be part of Patch Tuesday since we don't know what else this breaks. Updating the Docker base image would cause the same issue outside of CI.


services:
postgres:
Expand Down Expand Up @@ -67,7 +67,7 @@ jobs:
needs: tests

name: Build (and push) Docker image
runs-on: ubuntu-latest
runs-on: ubuntu-20.04

steps:
- uses: actions/checkout@v2
Expand Down
8 changes: 6 additions & 2 deletions .github/workflows/quick-start.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ on: [push]

jobs:
run:
runs-on: ubuntu-latest
runs-on: ubuntu-20.04
steps:
- uses: actions/checkout@v2
- name: Download docker-compose file
run: wget https://raw.githubusercontent.com/maykinmedia/objects-api/master/docker-compose-quickstart.yml -O docker-compose-qs.yml
- name: Overwrite the docker-compose file to get the "current" one
run: cp docker-compose-quickstart.yml docker-compose-qs.yml
- name: Start docker containers
run: docker-compose -f docker-compose-qs.yml up -d
- name: Wait until DB container starts
Expand All @@ -20,6 +23,7 @@ jobs:
run: |
curl_status=$(curl -w '%{http_code}' -o /dev/null -s http://localhost:8000/)
if [[ $curl_status != 200 ]]; then
printf "index page responds with %curl_status status" >&2
printf "Index page responds with ${curl_status} status.\r\n\r\n" >&2
curl -i http://localhost:8000
exit 1
fi
1 change: 1 addition & 0 deletions docker-compose-quickstart.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ services:
environment:
- DJANGO_SETTINGS_MODULE=objects.conf.docker
- SECRET_KEY=${SECRET_KEY:-1(@f(-6s_u(5fd&1sg^uvu2s(c-9sapw)1era8q&)g)h@cwxxg}
- ALLOWED_HOSTS=*
ports:
- 8000:8000
depends_on:
Expand Down
8 changes: 4 additions & 4 deletions src/objects/api/v1/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,12 +654,12 @@ components:
startAt:
type: string
format: date
description: Legal start date of the object record
description: Legal start date of the object record (inclusive)
endAt:
type: string
format: date
readOnly: true
description: Legal end date of the object record
description: Legal end date of the object record (exclusive)
registrationAt:
type: string
format: date
Expand Down Expand Up @@ -801,12 +801,12 @@ components:
startAt:
type: string
format: date
description: Legal start date of the object record
description: Legal start date of the object record (inclusive)
endAt:
type: string
format: date
readOnly: true
description: Legal end date of the object record
description: Legal end date of the object record (exclusive)
registrationAt:
type: string
format: date
Expand Down
8 changes: 4 additions & 4 deletions src/objects/api/v2/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,12 +742,12 @@ components:
startAt:
type: string
format: date
description: Legal start date of the object record
description: Legal start date of the object record (inclusive)
endAt:
type: string
format: date
readOnly: true
description: Legal end date of the object record
description: Legal end date of the object record (exclusive)
registrationAt:
type: string
format: date
Expand Down Expand Up @@ -894,12 +894,12 @@ components:
startAt:
type: string
format: date
description: Legal start date of the object record
description: Legal start date of the object record (inclusive)
endAt:
type: string
format: date
readOnly: true
description: Legal end date of the object record
description: Legal end date of the object record (exclusive)
registrationAt:
type: string
format: date
Expand Down
30 changes: 30 additions & 0 deletions src/objects/core/migrations/0028_auto_20230331_1239.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Generated by Django 2.2.28 on 2023-03-31 10:39

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("core", "0027_auto_20211203_1209"),
]

operations = [
migrations.AlterField(
model_name="objectrecord",
name="end_at",
field=models.DateField(
help_text="Legal end date of the object record (exclusive)",
null=True,
verbose_name="end at",
),
),
migrations.AlterField(
model_name="objectrecord",
name="start_at",
field=models.DateField(
help_text="Legal start date of the object record (inclusive)",
verbose_name="start at",
),
),
]
6 changes: 4 additions & 2 deletions src/objects/core/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,10 +98,12 @@ class ObjectRecord(models.Model):
encoder=DjangoJSONEncoder,
)
start_at = models.DateField(
_("start at"), help_text=_("Legal start date of the object record")
_("start at"), help_text=_("Legal start date of the object record (inclusive)")
)
end_at = models.DateField(
_("end at"), null=True, help_text=_("Legal end date of the object record")
_("end at"),
null=True,
help_text=_("Legal end date of the object record (exclusive)"),
)
registration_at = models.DateField(
_("registration at"),
Expand Down
4 changes: 2 additions & 2 deletions src/objects/core/query.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def filter_for_date(self, date):
return (
self.filter(records__start_at__lte=date)
.filter(
models.Q(records__end_at__gte=date)
models.Q(records__end_at__gt=date)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gave this some thought and my main concern was that this changed the filtering and not the data. Should the previous record not get an end date that is start date (of new record) - 1?

If the last record of an object, ends today. Is it valid today? According to the data it is but according to the filtering done here it doesn't match.

The docs are unclear what endsAt means (inclusive or exclusive). I need to ponder in this some more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed earlier today I'd read 'endsAt=29-03-2023' as that the object ends at 29-03-2023 00:00 (if the date is 29-03-2023 it's validity has ended, not that 29-03-2023 is the last day that the object still is valid). But GPT-4 disagreed with me:

Given the API specification, it is not explicitly mentioned whether the endAt date should be considered inclusive or exclusive. However, since the API deals with history and time periods, it would be reasonable to assume that the endAt date is inclusive. This means the object would still be considered valid on the 'endsAt' date.

It is indeed not explicit, but the alternative is that creating a new object and updating it on the same day without an explicit endAt means that multiple versions of the object will be valid on the current day. So the alternative (updating the object and updating the previous record's endAt to yesterday? I'm not sure that's possible via the API) seems less logical ( the API-test added shows the expected behaviour, I'm not sure how you would want to do this otherwise: https://github.com/maykinmedia/objects-api/pull/325/files#diff-c900604ca49840241e6d00d829c50ad83d98cbe15ce867417ffeac500322aba9 ).

Using datetimes instead of dates would make this explicit and the most sense in my eyes, I don't see why an object couldn't be valid for only a few seconds instead of a full day, but that's a different discussion.

Copy link
Member

@joeribekker joeribekker Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I explained another approach but I'm not sure if it introduces more magic than needed.

Using datetimes is a technical solution that doesn't take the intended use into account: An invoice date, date of birth, passport validity enddate... These are simply not datetimes.

So the alternative (updating the object and updating the previous record's endAt to yesterday? I'm not sure that's possible via the API) seems less logical

This can be done automatically just as the end date is now set automatically to the start date of the new record.

I just thought of something else but not sure if it covers the entire scope: if you have the record start and end date on the same day, and another record on the same start date as well, you must be correcting the previous record. I don't see any other use case. So, rather then fiddling with date filters we should maybe look at corrections.

| models.Q(records__end_at__isnull=True)
)
.distinct()
Expand Down Expand Up @@ -59,7 +59,7 @@ def filter_for_date(self, date):

"""
return self.filter(start_at__lte=date).filter(
models.Q(end_at__gte=date) | models.Q(end_at__isnull=True)
models.Q(end_at__gt=date) | models.Q(end_at__isnull=True)
)

def filter_for_registration_date(self, date):
Expand Down
47 changes: 47 additions & 0 deletions src/objects/tests/v2/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,53 @@ def test_filter_exclude_old_records(self):
data = response.json()["results"]
self.assertEqual(len(data), 0)

response = self.client.get(self.url, {"data_attrs": "diameter__exact__50"})

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()["results"]
self.assertEqual(len(data), 1)

def test_filter_exclude_old_records_issue_324(self):
"""
Filter and conflict resolution.

If record A ends at X and a newer record B starts at X, both match on
date X. Normally, conflict resolution returns only record B. However,
when you add a filter, it first finds the record matching this filter.
If the filter only results in record A, there are no conflicts and
record A is returned.
"""
record_old = ObjectRecordFactory.create(
data={"adres": {"straat": "Bospad"}},
object__object_type=self.object_type,
start_at=date.today() - timedelta(days=1),
end_at=date.today(),
)
record_new = ObjectRecordFactory.create(
data={"adres": {"straat": "Dorpsstraat"}},
object=record_old.object,
start_at=record_old.end_at,
)

response = self.client.get(
self.url, {"data_attrs": "adres__straat__exact__Bospad"}
)

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()["results"]
self.assertEqual(len(data), 0)

response = self.client.get(
self.url, {"data_attrs": "adres__straat__exact__Dorpsstraat"}
)

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()["results"]
self.assertEqual(len(data), 1)

def test_filter_date_field_gte(self):
record = ObjectRecordFactory.create(
data={"dateField": "2000-10-10"}, object__object_type=self.object_type
Expand Down
72 changes: 72 additions & 0 deletions src/objects/tests/v2/test_object_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,3 +380,75 @@ def test_updating_object_after_changing_the_startAt_value_returns_200(self, m):
response_updating_data_after_startAt_modification.status_code,
status.HTTP_200_OK,
)


@requests_mock.Mocker()
class ObjectApiTodayTests(TokenAuthMixin, APITestCase):
maxDiff = None

@classmethod
def setUpTestData(cls):
super().setUpTestData()

cls.object_type = ObjectTypeFactory(service__api_root=OBJECT_TYPES_API)
PermissionFactory.create(
object_type=cls.object_type,
mode=PermissionModes.read_and_write,
token_auth=cls.token_auth,
)

def test_update_object_issue_324(self, m):
mock_service_oas_get(m, OBJECT_TYPES_API, "objecttypes")
m.get(
f"{self.object_type.url}/versions/1",
json=mock_objecttype_version(self.object_type.url),
)
m.get(self.object_type.url, json=mock_objecttype(self.object_type.url))
today = date.today()

data = {
"type": self.object_type.url,
"record": {
"typeVersion": 1,
"data": {"adres": {"straat": "Bospad"}, "diameter": 30},
"startAt": today,
},
}
url = reverse("object-list")
response = self.client.post(url, data, **GEO_WRITE_KWARGS)

self.assertEqual(response.status_code, status.HTTP_201_CREATED)
object = Object.objects.get()

url = reverse("object-detail", args=[object.uuid])
data = {
"type": object.object_type.url,
"record": {
"typeVersion": 1,
"data": {"adres": {"straat": "Dorpsstraat"}, "diameter": 30},
"startAt": today,
},
}

response = self.client.put(url, data, **GEO_WRITE_KWARGS)
self.assertEqual(response.status_code, status.HTTP_200_OK)

object.refresh_from_db()
self.assertEqual(object.object_type, self.object_type)
self.assertEqual(object.records.count(), 2)
url = reverse("object-list")
response = self.client.get(url, {"data_attrs": "adres__straat__exact__Bospad"})

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()["results"]
self.assertEqual(len(data), 0)

response = self.client.get(
url, {"data_attrs": "adres__straat__exact__Dorpsstraat"}
)

self.assertEqual(response.status_code, status.HTTP_200_OK)

data = response.json()["results"]
self.assertEqual(len(data), 1)