lorenzodonini / ocpp-go

Open Charge Point Protocol implementation in Go
MIT License
275 stars 126 forks source link

2.0.1. types.SampleValue validation Value #151

Closed olga2323 closed 1 year ago

olga2323 commented 2 years ago

Hi there,

I discovered an issue with the validation of types.SampleValue.Value, required would exclude 0.0 Values.

type SampledValue struct {
    Value            float64           `json:"value" validate:"required"`                             // Indicates the measured value.
    Context          ReadingContext    `json:"context,omitempty" validate:"omitempty,readingContext"` // Type of detail value: start, end or sample. Default = "Sample.Periodic"
    Measurand        Measurand         `json:"measurand,omitempty" validate:"omitempty,measurand"`    // Type of measurement. Default = "Energy.Active.Import.Register"
    Phase            Phase             `json:"phase,omitempty" validate:"omitempty,phase"`            // Indicates how the measured value is to be interpreted. For instance between L1 and neutral (L1-N) Please note that not all values of phase are applicable to all Measurands. When phase is absent, the measured value is interpreted as an overall value.
    Location         Location          `json:"location,omitempty" validate:"omitempty,location"`      // Indicates where the measured value has been sampled.
    SignedMeterValue *SignedMeterValue `json:"signedMeterValue,omitempty" validate:"omitempty"`       // Contains the MeterValueSignature with sign/encoding method information.
    UnitOfMeasure    *UnitOfMeasure    `json:"unitOfMeasure,omitempty" validate:"omitempty"`          // Represents a UnitOfMeasure including a multiplier.
}

float64 and required will lead to something like Key: 'VAL.Value' Error:Field validation for 'Value' failed on the 'required' tag

Please let me know what do you think.

lorenzodonini commented 2 years ago

Hello, thanks for spotting this. It's tricky because in Go the "empty" value for numerical values is literally 0, so passing a 0 value will cause the validation to fail. I understand that passing a zero value is a perfectly valid use case though.

The only thing I can think of is to remove validation for this field altogether. The downside is that an endpoint may send a completely empty json for a sampled value -> it will be considered valid and the parsed value will be 0.0.

Clint-Mathews commented 1 year ago

Hey @lorenzodonini, was checking the documentation though it is a required field the value can be zero. As you said if removing that validation could cause an empty JSON to go in, can't we add custom validation checking if at least any fields have value? My suggestion might not the best to do but still, it feels like a possible workaround

Even though all the other fields are optional, if none of the fields including the value is empty, that could be an empty request. Considering a real-time charger scenario, there will be value against a parameter like voltage or power. So a custom validation to check if its empty could be a viable solution.

Referred document: OCPP-2.0.1_part2_specifiction.pdf from Open Charge Alliance.

If the solution looks good I can write up the MR/PR! Please let me know what you think :)

Also great work on the repo 😇

lorenzodonini commented 1 year ago

Hey @Clint-Mathews, the Value field is not a pointer, so it will always contain a value (fallback being zero). If an empty json actually came in, the parsed value would be 0. Validation always occurs before/after converting to/from json format, which sadly doesn't allow you to determine whether the json was empty.

The best idea that comes to mind is to make the field a pointer type, which could then be correctly validated. That would be a breaking change though. Or do you have any other concrete ideas for the validation? Totally open to suggestions.