TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
983 stars 309 forks source link

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

Closed jpmeijers closed 2 years ago

jpmeijers commented 4 years ago

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 https://github.com/TheThingsNetwork/lorawan-stack/blob/0478dd4d251c9ea5c7c4f2a1de0bb84093631bab/api/api.swagger.json#L9945-L9949

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

https://github.com/go-swagger/go-swagger/issues/2170 https://github.com/OAI/OpenAPI-Specification/issues/50

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

https://github.com/TheThingsNetwork/lorawan-stack/blob/0478dd4d251c9ea5c7c4f2a1de0bb84093631bab/api/identifiers.proto#L37-L48

Which refers to the DevAddr type, which uses Hex/Base16 to marshal to json https://github.com/TheThingsNetwork/lorawan-stack/blob/0478dd4d251c9ea5c7c4f2a1de0bb84093631bab/pkg/types/devaddr.go#L60-L61

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

johanstokking commented 4 years ago

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 commented 4 years ago

References #32 References #1982 References #2258

jpmeijers commented 4 years ago

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 commented 4 years ago

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

htdvisser commented 4 years ago

That's all done by https://github.com/grpc-ecosystem/grpc-gateway

johanstokking commented 4 years ago

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

neoaggelos commented 4 years ago

Hey @jpmeijers

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

johanstokking commented 4 years ago

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

neoaggelos commented 4 years ago

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 commented 4 years ago

https://github.com/TheThingsNetwork/lorawan-stack/issues/2297#issuecomment-609378470

neoaggelos commented 4 years ago

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 commented 4 years ago

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

johanstokking commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

References also https://github.com/TheThingsNetwork/lorawan-stack/issues/2798

jpmeijers commented 3 years ago

Oh wow a mess this gogo situation is causing

(oops, clicked close by accident)

jpmeijers commented 3 years ago

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 commented 3 years ago

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