search

lorenzodonini / ocpp-go

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

RequestStartTransactionRequest Structure fix #177

Closed yajan-singh closed 1 year ago

yajan-singh commented 1 year ago

IdTokenType in RequestStartTransactionRequest was structured incorrectly.

image
lorenzodonini commented 1 year ago

The protocol specification adds a Type suffix to basically every data type (see chapters 2 and 3). This notation wasn't the case in v1.6 btw. This library omits the suffix in most cases, because it is incredibly verbose and doesn't really add any value. For example The AdditionalInfoType in the posted screenshot was also shortened to just AdditionalInfo.

To be perfectly clear: this only affects the Go struct names, not the JSON fields itself (those are serialized according to specifications).

Is it a desired requirement to have the Type and EnumType suffix for all types? If so, then it would affect basically all data types in the 2.0.1 package. For consistency we should either keep them all as they are or change them all to include the Type and EnumType suffix. Changing them is quite a bit of work so I'd rather not do it, unless it is indeed a game-changer and provides real benefits.

yajan-singh commented 1 year ago

Thats makes sense, it is okay to remove the suffix but even then there the bug still exists. RequestStartTransactionRequest payload struct has an idToken field which should be of type idToken (struct | IdTokenType in the documentation) but instead points to idTokenType (enum | IdTokenEnumType in the documentation).

image image
lorenzodonini commented 1 year ago

I see the problem now. The suggested solution confused me, since it changed the whole naming. The types are correct, just the validation field is wrong. The reason this issue probably never showed up before is that the validation in this instance is ignored internally by the validation library.

I just merged a 1-liner fix: https://github.com/lorenzodonini/ocpp-go/pull/179, feel free to have a look. Afaict only the RequestStartTransactionRequest was affected by this.

lorenzodonini commented 1 year ago

Closing due to inactivity.