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.

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

Conversation

alextreme
Copy link
Member

@alextreme alextreme commented Mar 23, 2023

Resolves the same-date issue when retrieving objects

@alextreme alextreme requested a review from joeribekker March 23, 2023 16:32
@alextreme alextreme changed the title [#324] Regressiontests to avoid retrieval of old objects when querying using data_attrs [#324] Having startAt and endAt on today caused former objects to be shown Mar 23, 2023
@alextreme alextreme requested review from joeribekker and removed request for joeribekker March 28, 2023 22:03
@alextreme
Copy link
Member Author

Resolved failing 'ci' coordinate-tests by pinning testrunner to ubuntu-20.04 (ubuntu-latest switched to 22.04 somewhere last year).

quick-start should be working again after #327 is merged into master (as this action always runs against master for some reason).

@codecov-commenter
Copy link

Codecov Report

Merging #325 (cef7d70) into master (0c0f3d8) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #325      +/-   ##
==========================================
+ Coverage   94.72%   94.83%   +0.11%     
==========================================
  Files         129      131       +2     
  Lines        4490     4587      +97     
==========================================
+ Hits         4253     4350      +97     
  Misses        237      237              
Impacted Files Coverage Δ
src/objects/core/query.py 88.88% <ø> (ø)
src/objects/tests/v1/test_auth_fields.py 100.00% <ø> (ø)
src/objects/tests/v1/test_object_api_fields.py 100.00% <ø> (ø)
src/objects/tests/v2/test_auth_fields.py 100.00% <ø> (ø)
src/objects/tests/v2/test_object_api_fields.py 100.00% <ø> (ø)
src/objects/utils/serializers.py 96.70% <ø> (ø)
src/objects/__init__.py 100.00% <100.00%> (ø)
src/objects/accounts/models.py 91.66% <100.00%> (ø)
src/objects/api/constants.py 100.00% <100.00%> (ø)
src/objects/api/fields.py 84.61% <100.00%> (ø)
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -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.

@@ -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

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.

@joeribekker joeribekker force-pushed the issue-324-old-records-in-response branch 5 times, most recently from 36d6876 to bcf560f Compare April 3, 2023 13:20
@joeribekker joeribekker force-pushed the issue-324-old-records-in-response branch from bcf560f to 889ce02 Compare April 3, 2023 15:28
@joeribekker
Copy link
Member

I made a tag for this to resolve this temporarily with Alex' fix: 2.1.1-rt324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants