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

DateTime elements have seconds precision, optimized for fractions #2308

Merged
merged 3 commits into from
Apr 15, 2024
Merged

DateTime elements have seconds precision, optimized for fractions #2308

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e37f925
DateTime elements have seconds precision
ReimarBauer Mar 27, 2024
ab85d86
trying DateTime
ReimarBauer Mar 28, 2024
f5d82a3
trying 6 digits
ReimarBauer Mar 28, 2024
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
2 changes: 1 addition & 1 deletion mslib/mscolab/chat_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def get_messages(self, op_id, timestamp=None):
if timestamp is None:
timestamp = datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc)
else:
timestamp = datetime.datetime.strptime(timestamp, "%Y-%m-%d, %H:%M:%S %z")
timestamp = datetime.datetime.strptime(timestamp, "%Y-%m-%d, %H:%M:%S.%f %z")
Copy link
Collaborator

Choose a reason for hiding this comment

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

How flexible are we with this format? Is there some requirement on how it looks or is it just important that we use the same format for serialization and parsing between client and server respectively?

I am asking because I think this could just be .isoformat and .fromisoformat respectively. See: https://docs.python.org/3/library/datetime.html#datetime.datetime.isoformat, https://docs.python.org/3/library/datetime.html#datetime.datetime.fromisoformat.

With those there would be no potential for a format string mismatch and we would use a standardized representation that is unambiguous.

Copy link
Member Author

@ReimarBauer ReimarBauer Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We need to check with the timezone, isoformat would be prefered too.

a little headache I do have with the digits there, the testing setup needs 3 there. I have no idea, if the production system is happy or needs 6. This also the limitation here to milliseconds.

https://github.com/Open-MSS/MSS/pull/2308/files#diff-c721905a36cfba76833f0aa2f08229d63d2cbfa4ef0dcd2447a3e182880519b5R372

Copy link
Collaborator

Choose a reason for hiding this comment

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

Timezones shouldn't be an issue, they are properly handled by the isoformat methods. The number of zeros in that linked code line should be 6, since that would match what the %f format option also adds (6 zero-padded digits representing microseconds).

Copy link
Member Author

@ReimarBauer ReimarBauer Mar 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

correct
https://www.sqlite.org/lang_datefunc.html
In formats 4, 7, and 10, the fractional seconds value SS.SSS can have one or more digits following the decimal point. Exactly three digits are shown in the examples because only the first three digits are significant to the result, but the input string can have fewer or more than three digits and the date/time functions will still operate correctly. Similarly, format 12 is shown with 10 significant digits, but the date/time functions will really accept as many or as few digits as are necessary to represent the Julian day number.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can't follow. What are formats 4, 7, 10 and 12? Are you quoting from somewhere without referencing the source?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was because on many places the fractions symbolized by 3S

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK, this applies to the date and time related functions from SQLite. I don't think SQLAlchemy uses those though (it just dumps iso 8601 formatted strings to the database and reads those back as strings before parsing them, SQLite only ever sees text and doesn't have to parse it), so I don't think any of that applies to our use here.

messages = Message.query \
.filter(Message.op_id == op_id) \
.filter(Message.reply_id.is_(None)) \
Expand Down
2 changes: 1 addition & 1 deletion mslib/mscolab/file_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ def get_all_changes(self, op_id, user, named_version=False):
'comment': change.comment,
'version_name': change.version_name,
'username': change.user.username,
'created_at': change.created_at.strftime("%Y-%m-%d, %H:%M:%S %z")
'created_at': change.created_at.strftime("%Y-%m-%d, %H:%M:%S.%f %z")
}, changes))

def get_change_content(self, ch_id, user):
Expand Down
2 changes: 1 addition & 1 deletion mslib/mscolab/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,7 @@ def messages():
user = g.user
op_id = request.args.get("op_id", request.form.get("op_id", None))
if fm.is_member(user.id, op_id):
timestamp = request.args.get("timestamp", request.form.get("timestamp", "1970-01-01, 00:00:00 +00:00"))
timestamp = request.args.get("timestamp", request.form.get("timestamp", "1970-01-01, 00:00:00.000 +00:00"))
chat_messages = cm.get_messages(op_id, timestamp)
return jsonify({"messages": chat_messages})
return "False"
Expand Down
2 changes: 1 addition & 1 deletion mslib/mscolab/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ def get_message_dict(message):
"message_type": message.message_type,
"reply_id": message.reply_id,
"replies": [],
"time": message.created_at.strftime("%Y-%m-%d, %H:%M:%S %z")
"time": message.created_at.strftime("%Y-%m-%d, %H:%M:%S.%f %z")
}


Expand Down
3 changes: 2 additions & 1 deletion mslib/msui/mscolab_chat.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,8 @@ def load_all_messages(self):
data = {
"token": self.token,
"op_id": self.op_id,
"timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S %z")
"timestamp": datetime.datetime(1970, 1, 1,
tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S.%f %z")
}
# returns an array of messages
url = urljoin(self.mscolab_server_url, "messages")
Expand Down
2 changes: 1 addition & 1 deletion mslib/msui/mscolab_version_history.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ def load_all_changes(self):
changes = json.loads(r.text)["changes"]
self.changes.clear()
for change in changes:
created_at = datetime.strptime(change["created_at"], "%Y-%m-%d, %H:%M:%S %z")
created_at = datetime.strptime(change["created_at"], "%Y-%m-%d, %H:%M:%S.%f %z")
local_time = utc_to_local_datetime(created_at)
date = local_time.strftime('%d/%m/%Y')
time = local_time.strftime('%I:%M %p')
Expand Down
5 changes: 0 additions & 5 deletions tests/_test_mscolab/test_files_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
See the License for the specific language governing permissions and
limitations under the License.
"""
import time
import fs
import pytest

Expand Down Expand Up @@ -172,8 +171,6 @@ def test_get_all_changes(self):
with self.app.test_client():
flight_path, operation = self._create_operation(flight_path="V11")
assert self.fm.save_file(operation.id, "content1", self.user)
# we need to wait to get an updated created_at
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically it would still need to wait a microsecond. Practically, I don't think that will ever be an issue...

Copy link
Member Author

Choose a reason for hiding this comment

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

testing with sqlite would need a millisecond.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, microsecond is correct. As seen above the representation of DateTime in SQLite is stored with 6 decimal places, i.e. with a precision of one microsecond.

assert self.fm.save_file(operation.id, "content2", self.user)
all_changes = self.fm.get_all_changes(operation.id, self.user)
# the newest change is on index 0, because it has a recent created_at time
Expand All @@ -186,9 +183,7 @@ def test_get_change_content(self):
with self.app.test_client():
flight_path, operation = self._create_operation(flight_path="V12", content='initial')
assert self.fm.save_file(operation.id, "content1", self.user)
time.sleep(1)
assert self.fm.save_file(operation.id, "content2", self.user)
time.sleep(1)
assert self.fm.save_file(operation.id, "content3", self.user)
all_changes = self.fm.get_all_changes(operation.id, self.user)
previous_change = self.fm.get_change_content(all_changes[2]["id"], self.user)
Expand Down
4 changes: 0 additions & 4 deletions tests/_test_mscolab/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
See the License for the specific language governing permissions and
limitations under the License.
"""
import time
import pytest
import json
import io
Expand Down Expand Up @@ -234,7 +233,6 @@ def test_get_all_changes(self):
with self.app.test_client() as test_client:
operation, token = self._create_operation(test_client, self.userdata)
fm, user = self._save_content(operation, self.userdata)
time.sleep(1)
fm.save_file(operation.id, "content2", user)
# the newest change is on index 0, because it has a recent created_at time
response = test_client.get('/get_all_changes', data={"token": token,
Expand All @@ -252,8 +250,6 @@ def test_get_change_content(self):
with self.app.test_client() as test_client:
operation, token = self._create_operation(test_client, self.userdata)
fm, user = self._save_content(operation, self.userdata)
# we need to wait to get an updated created_at
time.sleep(1)
fm.save_file(operation.id, "content2", user)
all_changes = fm.get_all_changes(operation.id, user)
response = test_client.get('/get_change_content', data={"token": token,
Expand Down
11 changes: 7 additions & 4 deletions tests/_test_mscolab/test_sockets_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,12 @@ def test_get_messages(self):
assert messages[0]["text"] == "message from 1"
assert len(messages) == 2
assert messages[0]["u_id"] == self.user.id
timestamp = datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S %z")
timestamp = datetime.datetime(1970, 1, 1,
tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S.%f %z")
messages = self.cm.get_messages(1, timestamp)
assert len(messages) == 2
assert messages[0]["u_id"] == self.user.id
timestamp = datetime.datetime.now(tz=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S %z")
timestamp = datetime.datetime.now(tz=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S.%f %z")
messages = self.cm.get_messages(1, timestamp)
assert len(messages) == 0

Expand All @@ -221,7 +222,8 @@ def test_get_messages_api(self):
data = {
"token": token,
"op_id": self.operation.id,
"timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S %z")
"timestamp": datetime.datetime(1970, 1, 1,
tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S.%f %z")
}
# returns an array of messages
url = urljoin(self.url, 'messages')
Expand Down Expand Up @@ -257,7 +259,8 @@ def test_edit_message(self):
data = {
"token": token,
"op_id": self.operation.id,
"timestamp": datetime.datetime(1970, 1, 1, tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S %z")
"timestamp": datetime.datetime(1970, 1, 1,
tzinfo=datetime.timezone.utc).strftime("%Y-%m-%d, %H:%M:%S.%f %z")
}
# returns an array of messages
url = urljoin(self.url, 'messages')
Expand Down