search

Orange-OpenSource / its-client

This Intelligent Transportation Systems (ITS) MQTT client based on the JSon ETSI specification transcription provides a ready to connect project for the mobility (connected and autonomous vehicles, road side units, vulnerable road users,...). Let's connect your device or application to our Intelligent Transport Systems (ITS) platform!
MIT License
7 stars 8 forks source link

Schema: harmonize mobility messages in 2.0.0 versions #206

Open Hugues360 opened 2 weeks ago

Hugues360 commented 2 weeks ago

Fixes: #137

nbuffon commented 2 weeks ago

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 d4ccdee0 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 ?

ymorin-orange commented 2 weeks ago

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™...))

ymorin-orange commented 4 days ago

@Hugues360 I'm OK with the changes, good work!

However, I have some comments about the messages in the commit logs, but unfortunately GitHub does not allow commenting on commit logs... So, here are my concerns on a few of those commits:

Basically, a commit log should describe the change; it should explain it, and provide the rationale for the change. Of course, there are commits where the commit log would provide no additional value (e.g. the fix for the wording error does not need to be explained, it is so obvious). But when the "why" can't be answered just by looking at the commit, then the commit log is missing information.

Also, the title of the commit log should be imperative, not past tense. For example, it should be something like:

* `schemas: change field 'type' to 'message_type'`
* `schemas: remove unused field 'origin'`
* `schema/information: fix wording error`

and so on...

Hugues360 commented 4 days ago

@Hugues360 I'm OK with the changes, good work!

However, I have some comments about the messages in the commit logs, but unfortunately GitHub does not allow commenting on commit logs... So, here are my concerns on a few of those commits:

  • 0ca7bab schema/ : Changing field 'type' into 'message_type'

    • it is obvious when looking at the diff that the field is being renamed. What should be present in the commit log is the reason why it is being renamed, so that when in the future we are looking back at that commit, we do not have to wonder why we did that rename, or that we do not have to chase documentation that might have disapeared then...

  • 589bf7f schema: deleting origin field in schema having this field

    • ditto: the commit log should explain why we remove that field

  • 02ceb3a schema/status/status_schema_2-0-0.json: cellular and gnss are no longer required fields

    • ditto: the commit log should explain why those fields are no longer mandatory

Basically, a commit log should describe the change; it should explain it, and provide the rationale for the change. Of course, there are commits where the commit log would provide no additional value (e.g. the fix for the wording error does not need to be explained, it is so obvious). But when the "why" can't be answered just by looking at the commit, then the commit log is missing information.

Also, the title of the commit log should be imperative, not past tense. For example, it should be something like:

* `schemas: change field 'type' to 'message_type'`
* `schemas: remove unused field 'origin'`
* `schema/information: fix wording error`

and so on...

Thank you Yann for the comments. I have updated the commit messages

ymorin-orange commented 4 days ago

@nbuffon @mathieu1fb Last chance for a comment of your own! Hurry up if you still want to chime in! ;-)