Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type string byte in swagger file implies Base64 not Base16/Hex #2298

Closed
jpmeijers opened this issue Apr 5, 2020 · 19 comments · Fixed by #5627
Closed

Type string byte in swagger file implies Base64 not Base16/Hex #2298

jpmeijers opened this issue Apr 5, 2020 · 19 comments · Fixed by #5627
Assignees
Labels
compat/api This could affect API compatibility in progress We're working on it tooling Development tooling

Comments

@jpmeijers
Copy link

TL;DR

This issue is with Swagger, not with TTN. Swagger seems to only support Base64 for bytes, and no other formats like Base16.

Perhaps one should define it as just strings in the Swagger file, and not specifically as bytes.

Description

When one generate code according to the swagger file, the code will parse received DevAddr and DevEUI from TTN as Base64, while they are in fact Base16.

v3EndDeviceIdentifiers in the swagger file

"dev_addr": {
"type": "string",
"format": "byte",
"description": "The LoRaWAN DevAddr."
}

Compiles to the following go, which assumes Base64

	// The LoRaWAN DevAddr.
	// Format: byte
	DevAddr strfmt.Base64 `json:"dev_addr,omitempty"`

But the JSON I receive from the webhook looks like this, which is Base16/HEX

   "end_device_ids":{
      "device_id":"cricket-001",
      "application_ids":{
         "application_id":"jpm-crickets"
      },
      "dev_addr":"26011CE4"
   },

Some references

go-swagger/go-swagger#2170
OAI/OpenAPI-Specification#50

The proto definition where I assume the swagger definition is generated from

message EndDeviceIdentifiers {
option (gogoproto.populate) = false;
string device_id = 1 [(gogoproto.customname) = "DeviceID", (validate.rules).string = {pattern: "^[a-z0-9](?:[-]?[a-z0-9]){2,}$" , max_len: 36}];
ApplicationIdentifiers application_ids = 2 [(gogoproto.embed) = true, (gogoproto.nullable) = false, (validate.rules).message.required = true];
// The LoRaWAN DevEUI.
bytes dev_eui = 4 [(gogoproto.customtype) = "go.thethings.network/lorawan-stack/pkg/types.EUI64", (gogoproto.customname) = "DevEUI"];
// The LoRaWAN JoinEUI (AppEUI until LoRaWAN 1.0.3 end devices).
bytes join_eui = 5 [(gogoproto.customtype) = "go.thethings.network/lorawan-stack/pkg/types.EUI64", (gogoproto.customname) = "JoinEUI"];
// The LoRaWAN DevAddr.
bytes dev_addr = 6 [(gogoproto.customtype) = "go.thethings.network/lorawan-stack/pkg/types.DevAddr"];
}

Which refers to the DevAddr type, which uses Hex/Base16 to marshal to json

// MarshalJSON implements the json.Marshaler interface.
func (addr DevAddr) MarshalJSON() ([]byte, error) { return marshalJSONHexBytes(addr[:]) }

The same is true for the EUI type - it is also marshalled to HEX.

@johanstokking
Copy link
Member

Indeed, this is one of the early implementation choices we made, which breaks compatibility with JSONPB, i.e. what grpc-gateways interprets based on the protos from which the Swagger file is generated.

LoRaWAN specific types are indeed hex encoded in our API. We should somehow instruct the Swagger generator to reflect that.


PS please follow the issue template for future issues.

@johanstokking johanstokking added this to the Next Up milestone Apr 7, 2020
@johanstokking johanstokking added compat/api This could affect API compatibility tooling Development tooling labels Apr 7, 2020
@johanstokking
Copy link
Member

References #32
References #1982
References #2258

@jpmeijers
Copy link
Author

PS please follow the issue template for future issues.

When you browse the code on github, click on a line number, and then choose "Reference in new issue", the issue template is not filled in, so it's very difficult to keep to it using this workflow.

@johanstokking
Copy link
Member

@htdvisser can you give @neoaggelos some pointers to fix the Swagger file generation?

@htdvisser
Copy link
Contributor

@johanstokking
Copy link
Member

Right, so are we forking this to adapt for proto fields indicating non-standard types?

@neoaggelos
Copy link
Contributor

neoaggelos commented Apr 20, 2020

Hey @jpmeijers

May I ask what command you are using to generate code from the API swagger file?

@johanstokking
Copy link
Member

@neoaggelos you can try pasting our Swagger file here and use Generate Client in the top menu

@neoaggelos
Copy link
Contributor

This is what I am doing (also tried with the swagger-codegen-cli tool), and I get the DevAddr as a string field:

image

@jpmeijers
Copy link
Author

#2297 (comment)

@neoaggelos
Copy link
Contributor

neoaggelos commented Apr 20, 2020

Okay, so this is an issue with https://github.com/go-swagger/go-swagger, as the original Java swagger-codegen-cli tool seems to properly generate the field with type string.

And this is where the issue originates from https://github.com/go-swagger/go-swagger/blob/468a61360689c96f4e1a7350feeadf8eebb44b50/generator/formats.go#L142

@neoaggelos
Copy link
Contributor

How should we proceed with this? cc @htdvisser @johanstokking

@johanstokking
Copy link
Member

Fork and fix, I'm afraid.

Alternatively, run a script to "fix" the generated Swagger files. That might be cleaner until we remove our custom sauce in V4.

@jpmeijers
Copy link
Author

I'm running into this same issue, as well as other related field type mismatch issues, when generating Kotlin client code from the Swagger file. I also tried using the proto files to generate Kotlin classes, but after a long struggle the generated code is unusable. Currently it seems like I'll have to make do with the code generated from the swagger file, and then manually go through them all and fix the types.

./openapi-generator-cli.jar generate -g kotlin -i lorawan-stack/api/api.swagger.json
generates models which defines the EUI fields as type ByteArray, but what I receive in json is HEX encoded strings.

@johanstokking
Copy link
Member

Yep, the generated Swagger is plain wrong. We should either pick this up or remove Swagger for the time being, because currently this is useless.

@johanstokking
Copy link
Member

References also #2798

@jpmeijers
Copy link
Author

jpmeijers commented Dec 28, 2020

Oh wow a mess this gogo situation is causing

(oops, clicked close by accident)

@jpmeijers jpmeijers reopened this Dec 28, 2020
@jpmeijers
Copy link
Author

As a side note, I find the swagger file still very helpful. The reason is that swagger can by compiled into simple POJO or other simple classes with which can then parse the TTI json objects - with some slight manual changes to them. Up until now I have not been able to build clean (no grpc/protocol buffers) classes for unmarshalling json from the proto files.

@htdvisser htdvisser added the blocked This can't continue until another issue or pull request is done label Mar 2, 2021
@htdvisser
Copy link
Contributor

Blocked on #2798. We're going to do some more refactoring to our protos and generated code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat/api This could affect API compatibility in progress We're working on it tooling Development tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants