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

Reevaluate the spec vs event versioning schemes #87

Closed
e-backmark-ericsson opened this issue Oct 25, 2022 · 9 comments · Fixed by #90
Closed

Reevaluate the spec vs event versioning schemes #87

e-backmark-ericsson opened this issue Oct 25, 2022 · 9 comments · Fixed by #90
Milestone

Comments

@e-backmark-ericsson
Copy link
Contributor

It was apparent in the PR #86 that the way event vs spec versions are currently handled is problematic. The current versioning scheme requires that all event schemas are updated for any change to any individual event schema, since the spec version is updated for each event update and the spec version is part of each events individual schema. I believe that an individual event schema should not need to be updated unless "its own" contents are updated.

One way to handle this would be to not restrict the possible versions to set on the context.version property, but that has other drawbacks since then events could be sent with an invalid spec version set.

Alternatively we could remove the spec version from the context object, but it is not exactly know what consequences that would have to our SDKs etc.

@afrittoli
Copy link
Contributor

Thanks for bringing this up.

It was apparent in the PR #86 that the way event vs spec versions are currently handled is problematic. The current versioning scheme requires that all event schemas are updated for any change to any individual event schema, since the spec version is updated for each event update and the spec version is part of each events individual schema. I believe that an individual event schema should not need to be updated unless "its own" contents are updated.

One way to handle this would be to not restrict the possible versions to set on the context.version property, but that has other drawbacks since then events could be sent with an invalid spec version set.

Alternatively we could remove the spec version from the context object, but it is not exactly know what consequences that would have to our SDKs etc.

Removing the version for good would be backward incompatible and I would prefer to avoid that if possible.
The version field, in combination with the type, can be used by the SDK to download the schema which in turn can be used to validate the event, so I would propose removing the list of valid versions from the jsonschema .

afrittoli added a commit to afrittoli/cdevents-spec that referenced this issue Nov 15, 2022
Including the spec version enums in schemas is problematic, because
every time the spec is updated it implies that all the event schemas
must be updated as well which in turn requires the event type versions
to be updated too, which is confusing since not all events are updated
when a new spec version is made.

To solve that issue, we remove the enum from the version property, a let
it be a non-empty string. The SDKs can use the event type and version to
download the schema and validate events.

When a new spec release is made, the $id in the schema of all events
still have to be updated, but that should not cause the version on the
event type to be updated.

Fixes cdevents#87

Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
afrittoli added a commit to afrittoli/cdevents-spec that referenced this issue Nov 15, 2022
Including the spec version enums in schemas is problematic, because
every time the spec is updated it implies that all the event schemas
must be updated as well which in turn requires the event type versions
to be updated too, which is confusing since not all events are updated
when a new spec version is made.

To solve that issue, we remove the enum from the version property, a let
it be a non-empty string. The SDKs can use the event type and version to
download the schema and validate events.

When a new spec release is made, the $id in the schema of all events
still have to be updated, but that should not cause the version on the
event type to be updated.

Fixes cdevents#87

Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
afrittoli added a commit to afrittoli/cdevents-spec that referenced this issue Nov 15, 2022
Including the spec version enums in schemas is problematic, because
every time the spec is updated it implies that all the event schemas
must be updated as well which in turn requires the event type versions
to be updated too, which is confusing since not all events are updated
when a new spec version is made.

To solve that issue, we remove the enum from the version property, a let
it be a non-empty string. The SDKs can use the event type and version to
download the schema and validate events.

When a new spec release is made, the $id in the schema of all events
still have to be updated, but that should not cause the version on the
event type to be updated.

Fixes cdevents#87

Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
@e-backmark-ericsson
Copy link
Contributor Author

Backwards incompatible changes should of course be avoided, if possible, but I'm not sure how to solve this issue without removing it completely. If we leave the version field non-validated how can a consumer rely on it? Doesn't the consumer anyway need to parse the event using the correct event version instead of spec version?

@afrittoli
Copy link
Contributor

I see your point but there is actually still a way for consumers to check that the version in the context is compatible with the event type. Let's say I receive an event with the following context:

{
  "context": {
    "id": "8ec76402-9d85-42c8-8eb9-40f8d5a661f7",
    "source": "https://172.21.0.1:443",
    "timestamp": "2022-09-16T15:13:25.573904079Z",
    "type": "dev.cdevents.artifact.packaged.0.1.0",
    "version": "0.1.1"
  },

Using version and type I can fetch the schema "https://cdevents.dev/0.1.1/schema/artifact-packaged-event", check the default value for type and verify it matches. The default value for type is not in the schemas yet but I plan to add it as the next PR.

For the SDKs this means that they only need to know the spec version and they can find out the schemas from there.

If we were to drop the version field and keep only the version in the schema id, consumer would not be able to discover the schema anymore, so we would have two options:

  1. add a schema link to the context
  2. change the schema to be based on the event version instead of the spec one

(1) would be the same as the current situation, we would have a value that cannot be validated, and that can be used to fetch the schema for validation.
(2) would mean that we drop the version schema completely from the schemas and use it only for git tags and browsing the spec.

@e-backmark-ericsson
Copy link
Contributor Author

Ok. I believe I understand your line of thought there. In Eiffel we do as proposed in 2. above, but I would be fine trying out to have the spec version part of the context, as long as it doesn't require all event schemas to be updated. It's a bit hard to visualize this so I'd propose we proceed with the related PR and also the one adding the fixed version number to the context.type, so we can more clearly see how things turn out before we definitely keep it that way for spec release 0.2.

@afrittoli
Copy link
Contributor

Thank you, it sounds good - after this, we can still decide to remove version and change the ids if we are not satisfied with approach (1).

@e-backmark-ericsson
Copy link
Contributor Author

We might anyway want to provide an option to include a schema link in the context, since some organizations might want to restrict the event schemas more than what we do in the spec itself, and provide references to their own schema registries overriding the ones provided by CDEvents, but that could be a later addition to the protocol.

@afrittoli
Copy link
Contributor

We might anyway want to provide an option to include a schema link in the context, since some organizations might want to restrict the event schemas more than what we do in the spec itself, and provide references to their own schema registries overriding the ones provided by CDEvents, but that could be a later addition to the protocol.

Would you mind creating an issue for that? If we consider different schemas, I think we'll need to define a method to test whether a specific schema is CDEvents compliant. I would not want to have event sent with CDEvents types which miss mandatory attributes.

@e-backmark-ericsson
Copy link
Contributor Author

Of course all CDEvents need to conform to the CDEvents schemas. But if some organizations wishes to restrict those schemas even further I believe we need to provide a possibility to do so. All events should still include the correct CDEvents versions fields, but it might be so that we should allow a schema url to be provided by the users and not dictated by the CDEvents protocol itself.

afrittoli added a commit that referenced this issue Nov 17, 2022
Including the spec version enums in schemas is problematic, because
every time the spec is updated it implies that all the event schemas
must be updated as well which in turn requires the event type versions
to be updated too, which is confusing since not all events are updated
when a new spec version is made.

To solve that issue, we remove the enum from the version property, a let
it be a non-empty string. The SDKs can use the event type and version to
download the schema and validate events.

When a new spec release is made, the $id in the schema of all events
still have to be updated, but that should not cause the version on the
event type to be updated.

Fixes #87

Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
Repository owner moved this from Todo to Done in CDEvents Releases Nov 17, 2022
@e-backmark-ericsson
Copy link
Contributor Author

Related issue about introducing a schema uri in the context object: #91

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

Successfully merging a pull request may close this issue.

2 participants