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

~.000008sec difference between 'created' and 'updated' field in API #4411

Closed
perezfr opened this issue Mar 7, 2017 · 4 comments
Closed

~.000008sec difference between 'created' and 'updated' field in API #4411

perezfr opened this issue Mar 7, 2017 · 4 comments

Comments

@perezfr
Copy link

perezfr commented Mar 7, 2017

Steps to reproduce

  1. Query the api 'search' function with uri as 'https://www.commonsense.org/education/privacy/blog/digital-redlining-access-privacy'

Expected behaviour

'created' and 'updated' field should not be different for annotations that aren't updated.

Actual behaviour

There are messages with ~.000008sec difference between 'created' and 'updated' field

Browser/system information

Using the API search feature https://hypothes.is/api/search

Additional details

attached csv of list of messages affected by the issue
augCreateUpdate.txt

@seanh
Copy link
Contributor

seanh commented Mar 20, 2017

Thanks @perezfr , I can confirm that I can reproduce this bug.

@chdorner @fatbusinessman Should we fix this issue?

When you create an annotation, I guess the model code calls utcnow() once for created and then again for updated, rather than calling it once and assigning the same one value to both: https://github.com/hypothesis/h/blob/master/src/memex/models/annotation.py

The result is that, for an annotation that has never been edited, created and updated are not exactly the same. This is wrong and I can in fact see it being a pain (for example if you want to produce a list of all annotations that have been edited, by finding those with different created and updated dates).

The obvious fix to me would be for Annotation to have an __init__() method that calls utcnow() once and sets it as the value of both created and updated, although I anticipate objections because I understand that people don't like adding code, especially not __init__() methods, to model classes. This particular __init__() method would not change the API of creating Annotation objects though.

Perhaps there is some declarative way to tell sqlalchemy to set updated to the same value that was generated for created? But I don't know if off the top of my head.

Or we find the user code outside of the model class and have it run utcnow() once and pass in the one value for both created and updated. This seems wrong to me (I would go with the __init__() method) but it would work as long as we don't grow more lines of code that create Annotations in the future and forget to do this correctly there as well.

Or we decide not to fix this.


As for existing annotations in the db that have never been edited but that have different created and updated times, if we wanted to fix those retrospectively all I can think of is a database migration that looks for annotations whose created and updated are within some tiny fraction of a second delta of each other and changes them to be the same.

@chdorner
Copy link
Contributor

Looking at the code this actually looks like a bug, the code does try to set it to the same timestamp: here and here.
In general, the fix should happen outside the model though, because h.storage.create_annotation is the only code path that is responsible for creating an annotation, so it should happen in there.

We should look into fixing this for new annotations, I'm not sure how if it's worth the effort for old annotations. It might be fairly straightforward if we are okay with making the decision that if the updated is not more than one second ahead of created than we set the updated timestamp to the same as created.

@perezfr
Copy link
Author

perezfr commented Mar 28, 2017

The issue only seems to affect annotation data from before August/September 2016. Newer records don't seem to be affected by this issue.

@seanh
Copy link
Contributor

seanh commented May 22, 2023

I think this was fixed a long time ago

@seanh seanh closed this as completed May 22, 2023
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

No branches or pull requests

3 participants