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

Schema: harmonize mobility messages in 2.0.0 versions #206

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

Conversation

Hugues360
Copy link
Collaborator

Fixes: #137

@Hugues360 Hugues360 added the Schema JSON schema files label Nov 12, 2024
@Hugues360 Hugues360 added this to the Sprint 4 milestone Nov 12, 2024
@Hugues360 Hugues360 self-assigned this Nov 12, 2024
@nbuffon
Copy link
Member

nbuffon commented Nov 13, 2024

For now SDK and application do not allow to use a specific version of a schema, and then are considered using the last version by default
Changes like renaming a field (see commit d4ccdee for example) would make the use of this new version (the would then be last one) incompatible with the implementation, like the Rust one would no longer be able to deserialize any incoming message
Introducing such changes should also provide the changes in the implementation in each languages

@ymorin-orange @mathieu1fb any opinion on this ?

@@ -31,7 +31,7 @@
"version": {
"type": "string",
"description": "json message format version",
"const": "1.99.99"
"const": "2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

No need to go through 1.99.99, just put 2.0.0 directly in the new schema, you can use this commit to fixup the original one introducing 2_0_0 file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In fact, seen with Yann, it's a way to have only the right version number (2.0.0 in this case) in the very last commit including all the updates. If someone pull an "intermediate" commit, before the last one, he won't have the right version in the schema but a version with .99 indicating it's a developing version.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this avoids cases where two files, at two different revisions, have the same version:

  1. if we add/delete/rename fields while still keeping the old version number, then we have multiple schemas with the old version, and it is not easy to find which is the correct one;
  2. if we first change the version, then the same is true, but with the new version.

We can't really rely on the filenames, because filenames can change; we can only rely on the version in the schemas themselves. Thus, it is important to denote that a schema is not stable, and that it should not be used; this is the role of the .99 version, which clearly inidicates that.

When discussing this with @Hugues360, I subtly hinted that we should have documentation that explains our process at maintaining those schmas. Way too subtly, maybe... ;-) More generally, though, we should have a documentation that explains the versioning scheme for each component (java, python, rust, schemas).

@ymorin-orange
Copy link
Member

ymorin-orange commented Nov 15, 2024

For now SDK and application do not allow to use a specific version of a schema, and then are considered using the last version by default Changes like renaming a field (see commit d4ccdee for example) would make the use of this new version (the would then be last one) incompatible with the implementation, like the Rust one would no longer be able to deserialize any incoming message Introducing such changes should also provide the changes in the implementation in each languages

@ymorin-orange @mathieu1fb any opinion on this ?

  1. First and foremost, the commit log should explain the reasoning for this renaming, if not to avoid such comment, but at the very least for reference for the future, when we all have long forgotten about this.
  2. Our applications are for now only emitting a single version (the current-latest one) of each schema; just updating the schemas would not magically make all those applications emit the new versions, and thus no message of a newer version would be received by applications. So it is totally acceptable to create new schemas with an updated version in which we change the fields; that is explicitly the reason why we do have a version in them.
  3. If an implementation can't cope with versions it does not recognise, but just ignores those mesages, that's perfectly acceptable too, and is again the reason for the versioning. If however an implementation crashes, then that's a bug in that implementation, and should be fixed (to at least ignore unknown messages, or ignore versions of known messages); indeed, once we start updating an application, then newer messages will be emitted, and that will have to be handled (in any suitable way: ignore or use) by applications. Note the fifth point below, too.
  4. Updating applications to use those newer schemas should be scheduled as son as the newer schemas are validated.
  5. Currently, we do not validate the messages against the schemas before we use those messages, which opens up the door to problems in interpreting the messages, and this is the crux of the issue. When we add validation to our applications (via the SDK), then it will be easier to have a mixed-versions interchange of schemas, where applications would just ignore messages that do not match any schema (and thus any version of such schema) they know of.

(Note: I would be surprised if our python apps would have no issue with newer schemas as well... Although they mostly creates them and do not pasrse them for now, so the issue would be largely mitigated already (Famous Last Words™...))

@@ -26,7 +26,7 @@
"description": "json message format version",
"const": "1.99.99"
},
"instance_id": {
"source_uuid": {
"description": "unique id all other the world for a server",
Copy link
Member

@ymorin-orange ymorin-orange Nov 15, 2024

Choose a reason for hiding this comment

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

Could you also fix this error (note: it exists in many schemas): s/all other/all over/, please?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that Yann meant to replace instance_id by source_uuid as they do not represent the same information. He was referring to the spelling mistakes in the description.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that Yann meant to replace instance_id by source_uuid as they do not represent the same information. He was referring to the spelling mistakes in the description.

  1. Indeed, I was speaking about the typo, and Hugues did fix it in the new iteration.
  2. We still want to harmonise the top-level fields so that all messages still contain the same "header":
    • message_type: the type of message this is
    • version: the version of the schema that applies
    • timestamp: the UNIX timestamp the message was created at
    • source_uuid: the universally unique ID of the source that emitted the message
    • origin: the (type of) entity that emitted the message
    • message: the JSON-translation of the ETSI message

So far, only the schema for information message defines an instance_id. If that is not the same thing as source_uuid, then we indeed must add source_uuid and keep instance_id.

Note that CAM, for example, has a message.station_id. Is that the same thing as the instance_id from the information message, or is it yet something else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I understand it, the instance_id is the ID of a mobility server, but the source_uuid could be the one of the authority sending the information message, it is not necessarily the same entity.

Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand it, [...] it is not necessarily the same entity.

OK, then we need the two fields. Are they both mandatory, then?

And BTW, is this an ETSI-derived message, or our own invention?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, they are both mandatory, and it is a message of our invention.

@Hugues360 Hugues360 force-pushed the yhov/mobility_schema branch 2 times, most recently from 1f67ffa to da0eeb7 Compare November 19, 2024 16:28
…directories

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
…tus, neighbourhood, region, srem and ssem messages

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
…mon ones first, alphabetical order)

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
…er required fields

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
…, status, neighbourhood, region, srem and ssem messages

Signed-off-by: Hugues Oudeville <hugues.oudeville@orange.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Schema JSON schema files
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Define mobility message contracts
4 participants