bugfix_value_type_of_dataschema

[timmyb32r]

it's improved copy of #985 - I made typo and couldn't fix previous branch

[duglin]

From the issue:

I occurred this problem when our users made correct cloudevents message with some non-golang sdk, and send it to our code.

can you elaborate on this? It appears that even with your change the URI will be serialized as a string, right? So why did things break when talking with a non-golang sdk?

[timmyb32r]

It appears that even with your change the URI will be serialized as a string, right?

no, with my change DataSchema becames 'URI'. there are next code in project cloudevents/sdk-go:

func attributeFor(v interface{}) (*pb.CloudEventAttributeValue, error) {
    vv, err := types.Validate(v)
    //...
    switch vt := vv.(type) {
    case string:
        attr.Attr = &pb.CloudEventAttributeValue_CeString{
            CeString: vt,
        }
    case types.URI:
        attr.Attr = &pb.CloudEventAttributeValue_CeUri{
            CeUri: vt.String(),
        }
    //...
    return attr, nil
}

and types.Validate:

func Validate(v interface{}) (interface{}, error) {
    switch v := v.(type) {
    case bool, int32, string, []byte:
        return v, nil // Already a CloudEvents type, no validation needed.
    case *url.URL:
        if v == nil {
            break
        }
        return URI{URL: *v}, nil

so, in trunk now next code:
container.Attributes[dataschema], _ = attributeFor(e.DataSchema())
DataSchema() returns a string, and then string goes to attributeFor, and Validate returns string, and then

    case string:
        attr.Attr = &pb.CloudEventAttributeValue_CeString{
            CeString: vt,
        }

so, trunk creates type CeString

in my PR this becames:

dataSchemaStr := e.DataSchema()
uri, err := url.Parse(dataSchemaStr) // https://pkg.go.dev/net/url#Parse - returns *URL
if err != nil {
    return nil, fmt.Errorf("failed to url.Parse %s: %s", dataSchemaStr, err)
}
container.Attributes[dataschema], _ = attributeFor(uri) // passes *URL, which then in func 'Validate' becames type URI ne

so, *URL goes to 'attributeFor', and types.Validate make URI from *URL, and then in 'attributeFor' executes this branch:

    case types.URI:
        attr.Attr = &pb.CloudEventAttributeValue_CeUri{
            CeUri: vt.String(),
        }

This way DataSchema in trunk now CeString, and becomes CeUri. And documentations says 'DataSchema' must be URI, not string: https://github.com/cloudevents/spec/blob/main/cloudevents/spec.md#dataschema

---

also I'm fixing FromProto
and change

vs, _ := v.(string)
e.SetDataSchema(vs)

to

vs, _ := v.(types.URI)
e.SetDataSchema(vs.String())

so, FromProto in my PR withs with DataSchema==URI instead of string, what accordings to documentation: https://github.com/cloudevents/spec/blob/main/cloudevents/spec.md#dataschema

[timmyb32r]

So why did things break when talking with a non-golang sdk?

my colleagues built message where DataSchema is URI, and cloudevents/sdk-go couldn't decode it, bcs it failed here: https://github.com/cloudevents/sdk-go/blob/main/binding/format/protobuf/v2/protobuf.go#L254

according to spec here must be URI, and cast to string makes an error - and this error ignores here via underlining, and empty string assigns to DataSchema field. I've got situation, when I have correct cloudevents message (according specification), and go-sdk corrupt the data, substitute empty value insteam of field value.

@duglin

[timmyb32r]

up

@duglin

[duglin]

Sorry for the delay - and thanks for the explanation.
/LGTM

[duglin]

ping @embano1 and @lionelvillard for another set of eyes

[timmyb32r]

ping @embano1 and @lionelvillard

[embano1]

Just some minor things/question

[embano1]

nit: use error wrapping

Suggested change

	return nil, fmt.Errorf("failed to url.Parse %s: %s", dataSchemaStr, err)
	return nil, fmt.Errorf("failed to url.Parse %s: %w", dataSchemaStr, err)

[embano1]

question: assuming this is guaranteed to be of type types.URI here due to your URI check earlier (no panic)?

[timmyb32r]

good question. I've checked:
vs, _ := v.(types.URI) will return default constructed types.URI{}, and further called mechod .String() will return an empty string - so, it won't panic

[embano1]

nit: can we use a proper URI for this test?

[embano1]

LGTM

[embano1]

Can you please rebase onto main and also sign your commits or whatever is causing the DCO to fail?

[timmyb32r]

@duglin @embano1 what we should also do to merge it? Idk why these last checks (Build/CloudEvents/Unit Test) wont pass :(