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

Fix the schemas for custom data #79

Merged
merged 1 commit into from
Oct 19, 2022

Conversation

afrittoli
Copy link
Contributor

Changes

The current schema only includes string/base64 which is used for non JSON data.
customData may include either JSON (object) or base64 encoded content (string).

Fix one of the examples in the spec that was possibly misleading.

Signed-off-by: Andrea Frittoli andrea.frittoli@gmail.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

The current schema only includes string/base64 which is used for
non JSON data. customData may include either JSON (object) or
base64 encoded content (string).

Fix one of the examples in the spec that was possibly misleading.

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

In Eiffel we have the following design rule:

Do not use variable key names: For purposes of automated validation, analysis and search, custom key names shall be avoided. Consequently, for custom key-value pairs { "key": "customKeyName", "value": "customValue" } shall be used instead of { "customKeyName": "customValue" }.

And our customData is defined like this in json schema:

      "customData": {
          "type": "array",
          "items": {
            "type": "object",
            "properties": {
              "key": {
                "type": "string"
              },
              "value": {}
            },
            "required": [
              "key",
              "value"
            ],
            "additionalProperties": false
          }
        }

Do you think that should be a valid design rule for CDEvents as well?

It would then be something like this instead:

"customData": {
    "oneOf": [
        {
            "type": "string",
            "contentEncoding": "base64"
        },
        {
            "type": "array",
            "items": {
                "type": "object",
                "properties": {
                    "key": {
                        "type": "string"
                    },
                    "value": {}
                },
                "required": [
                    "key",
                    "value"
                ],
                "additionalProperties": false
            }
        }
    ]
}

@afrittoli
Copy link
Contributor Author

In Eiffel we have the following design rule:

Do not use variable key names: For purposes of automated validation, analysis and search, custom key names shall be avoided. Consequently, for custom key-value pairs { "key": "customKeyName", "value": "customValue" } shall be used instead of { "customKeyName": "customValue" }.

And our customData is defined like this in json schema:

      "customData": {
          "type": "array",
          "items": {
            "type": "object",
            "properties": {
              "key": {
                "type": "string"
              },
              "value": {}
            },
            "required": [
              "key",
              "value"
            ],
            "additionalProperties": false
          }
        }

Do you think that should be a valid design rule for CDEvents as well?

It would then be something like this instead:

"customData": {
    "oneOf": [
        {
            "type": "string",
            "contentEncoding": "base64"
        },
        {
            "type": "array",
            "items": {
                "type": "object",
                "properties": {
                    "key": {
                        "type": "string"
                    },
                    "value": {}
                },
                "required": [
                    "key",
                    "value"
                ],
                "additionalProperties": false
            }
        }
    ]
}

Thanks, @e-backmark-ericsson - yeah I can see how that could make sense - k8s has a somewhat similar policy about using arrays instead of maps in the API. We could then customData added from different sources, for instance in a case where a CDEvent is sent as an aggregation of multiple incoming events.

I'm not sure how that would map to the non-JSON content case. We would probably have to allow for some of the items in the array to carry base64 encoded data, with their own customDataContentType.

The scope of this PR is only to align the schema to what the spec says today, which will allow me to switch the SDK tests to use the schemas from the spec repo.

I would propose we treat your proposal as a follow-up item, would that be ok?

@e-backmark-ericsson
Copy link
Contributor

e-backmark-ericsson commented Oct 19, 2022

Sure. My proposal could be considered for 0.2. Will you make a new issue about that, also mentioning the K8s policy, or do you want me to write an issue mostly based on the Eiffel way of defining custom data? I would like to see that new issue written before approving this PR, to make sure we don't forget about it.

@afrittoli
Copy link
Contributor Author

Issue created: #81

@afrittoli afrittoli merged commit 1640f65 into cdevents:main Oct 19, 2022
@afrittoli afrittoli linked an issue Mar 13, 2023 that may be closed by this pull request
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

Successfully merging this pull request may close these issues.

Payload and Versioning
2 participants