cdevents / sdk-java

Java SDK for CDEvents
Apache License 2.0
5 stars 6 forks source link

Validating CDEvent fails if not setting Optional fields #51

Closed rjalander closed 1 year ago

rjalander commented 1 year ago

As per the spec for most of the events subject.content filds are optional, but validating the event against jsonschema fails if not setting these optional fields when creating an event,

An example pipelineRunQueued events fails without setting these optional fields source, pipelineName, url CDEvent validation failed with errors [ $.subject.source: null found, string expected, $.subject.content.pipelineName: null found, string expected, $.subject.content.url: null found, string expected ]

rjalander commented 1 year ago

Is this something we need to allow string and null(for optional fields) something like "type": ["string","null"] in the jsonschema, wondering how it is handled in sdk-go I don't see it's having default values for optional fields to pass this schema validation?

afrittoli commented 1 year ago

I don't think this is something in the jsonschema.

In the go sdk i had a similar issue because I wasn't using pointers in the types, and the JSON renderer was than producing the zero value for the types, which is not allowed by the schema i.e. an optional field should either not be there at all or, if it's there it should meet the schema requirements.

The Java libraries will most likely behave differently from the go ones, but it might be an issue in a similar kind of area.

rjalander commented 1 year ago

If I change the default values to empty for optional fields, the validation goes fine. I think the same way it is handled in sdk-go with omitempty, If I am not wrong.

afrittoli commented 1 year ago

Yes indeed. Specifically for go, omitempty only with pointers.

rjalander commented 1 year ago

Also, comparing the datatypes of the fields for the events in jsonschema(/go-sdk) vs spec, most of the types in jsonschema are string but they are defined as URI, Enum, Purland URI-Reference. I think it should follow the rules from spec right?, In that case all the default values will be set to null

afrittoli commented 1 year ago

Thanks @rjalander - you are right, the spec is more specific than the JSONSchema today in a few places.

For URI and URI reference, that's something that was hopefully fixed in https://github.com/cdevents/spec/pull/116 - so that should solve itself once you work on the latest version.

For enums, AFAIK everything which is an enum is defined so in the schema, if you have examples where that is not the case please let me know, so we can fix that.

For purls, I don't think it's possible to enforce that format today in JSONSchemas. That's something we can work on together with the purl and JSONSchema communities. In the meanwhile, we have a gap between what can be enforced in the schema and what is documented in the spec. When we work on generating the spec docs from the schema, we'll need a way to handle that. In terms of SDKs, for the golang SDK for now I have a hack in place, it looks for fields called "artifactId" and it enforces the "purl" format for them. In future we may need a more general solution for that.

rjalander commented 1 year ago

I think fixing the format of specific type as per https://github.com/cdevents/spec/pull/116 will solves the problem If invalid input value is set into the field, but the problem in Java-SDK still exists If the value of an object itself is unset as it can be optional. In this case the type mentioned as String is not valid in case of Java object is not set, that is default to null.

Can we expect the type for all the optional fields in the jsonschema as an array of string and null( "type": ["string","null"] )?, As in Java there is no replacement for omitempty that is used in go, rather suggesting to expect the null type.

Please share your comments on the same.

afrittoli commented 1 year ago

That would weaken the schema, it would say that we allow for the field to be set and empty, which I would prefer to avoid.

Isn't there any option in the java library that renders to JSON to not render unset fields?

rjalander commented 1 year ago

Ok, yes we can use jackson library when serializing the object to exclude optional unset/null fields, before validating against jsonschema. This way the resultant CDEvent data will not have any optional fields itself when unset, If that is fine.(This might be different when comparing with the go-sdk, the optional fileds set to empty when unset in the CDEvent data) ?

afrittoli commented 1 year ago

Empty optional fields are not rendered in the go SDK, or else the json would fail validation, like it happened for the java sdk

rjalander commented 1 year ago

Ok, will go with this and create a PR for not to render optional fields with null. Thank you.

rjalander commented 1 year ago

Closing, as the PR to fix the issue is merged. https://github.com/cdevents/sdk-java/pull/55