Skip to content

config: Handle null attributes as invalid #6629

@pellared

Description

@pellared
Member

null attribute should be considered as an invalid input for ParseYAML (and when parsing JSON as well) as we already have test cases like invalid nil value.

Originally posted by @pellared in #6603

CC @codeboten

Activity

mikeblum

mikeblum commented on Jan 16, 2025

@mikeblum
Contributor

Can I pick this up? @pellared

added a commit that references this issue on Jan 18, 2025
linked a pull request that will close this issue on Jan 20, 2025
mikeblum

mikeblum commented on Mar 13, 2025

@mikeblum
Contributor

After digging pretty deeply into the YAML spec its unclear to me if we want to be writing custom parsers to error on different cases such as:

attr: null
attr: []
attr: 0
attr:

map:
  -

imo the current behaviour of parseYAML ignoring these attributes entirely is correct but I defer to @pellared on what the correct behavious should be.

pellared

pellared commented on Mar 13, 2025

@pellared
MemberAuthor

We should consistently "ignore" invalid values or return an error. I think that if something is not compliant with the scheme then it should return an error.

Moreover, from https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#parse:

Parse SHOULD return an error if [...] The parsed file content does not conform to the configuration model schema.

its unclear to me if we want to be writing custom parsers to error on different cases such as:

This is my expectation.

MrAlias

MrAlias commented on Mar 13, 2025

@MrAlias
Contributor

its unclear to me if we want to be writing custom parsers to error on different cases such as:

This is my expectation.

Wouldn't this mean we would completely rewrite a yaml parsing library? That does not seem like something we want to maintain.

pellared

pellared commented on Mar 13, 2025

@pellared
MemberAuthor

Wouldn't this mean we would completely rewrite a yaml parsing library?

I mean add additional validation which is unfortunately not handled by github.com/atombender/go-jsonschema. It should not be a rewrite. It seems we already doing it in config_json.go and config_yaml.go.

However, probably it would be a lot better to contribute to https://github.com/omissis/go-jsonschema (or at least create issues).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area: file-configRelated to file-based configurationenhancementNew feature or request

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

    Participants

    @mikeblum@pellared@MrAlias

    Issue actions

      config: Handle null attributes as invalid · Issue #6629 · open-telemetry/opentelemetry-go-contrib