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

Publish clean API protos #2297

Closed
jpmeijers opened this issue Apr 5, 2020 · 10 comments
Closed

Publish clean API protos #2297

jpmeijers opened this issue Apr 5, 2020 · 10 comments
Labels
compat/api This could affect API compatibility

Comments

@jpmeijers
Copy link

Summary

DEVELOPMENT.md describes how one can generate the API definitions for JavaScript. It however does not describe how to do this for other languages. Due to external dependencies in the proto files they can't simply be compiled by protoc to any language one wants.

It would be great if the proto files did not have any foreign dependencies so that one can simply compile them into the language you want.

On TTNv2 if you wanted golang type struct definitions you could simple copy them from the repo here: https://github.com/TheThingsNetwork/ttn/tree/develop/core/types
This is not the case with V3.

Why do we need this?

Allow building services where the webhooks can send data, being able to parse the messages correctly according to the central definition (proto file).

What is already there? What do you see now?

The proto files are there, and I see a command to run to give me api definitions for Javascript, not other languages.

What is missing? What do you want to see?

A generic way to compile the proto files into any supported language. I also want type definitions for go, but if the protos can compile I can compile it to go myself.

Environment

N/A

How do you propose to implement this?

Clean up the proto definitions in the api directory so that they do not depend on foreign dependencies, OR document how one can compile them for any supported language, not just JS.

Can you do this yourself and submit a Pull Request?

Been trying to figure this out, but because the tooling uses mage, and a lot of custom build scripts were written to compile the codebase and to export the JS definitions, this is rather complicated to understand.

@jpmeijers
Copy link
Author

It seems like a workaround would be to generate the go model from the swagger definition:

swagger generate model -f api.swagger.json --skip-validation

This however feels counter intuitive, unless the swagger definition is the primary definition and not the proto files.

@jpmeijers jpmeijers changed the title Publish API definitief Publish API definitions Apr 5, 2020
@johanstokking
Copy link
Member

Thanks for the report. Indeed, the proto files in api are full of options that we use primarily in this particular repository.

We have some ideas to generate clean protos which would go to a separate repository.

Indeed, we have the Swagger which should act as a reference. So we technically publish the API definitions that way; I'll keep this issue open until we have clean protos.

@johanstokking johanstokking changed the title Publish API definitions Publish clean API protos Apr 7, 2020
@johanstokking johanstokking added this to the Backlog milestone Apr 7, 2020
@johanstokking johanstokking added the compat/api This could affect API compatibility label Apr 7, 2020
rvolosatovs added a commit to rvolosatovs/lorawan-stack-fork that referenced this issue Sep 15, 2020
@jpmeijers
Copy link
Author

Still running into this same issue. Trying to add V3 webhook support to my systems, I'm struggling to find a definitive protocol specification of how the json I will receive should look. The definition should be https://github.com/TheThingsNetwork/lorawan-stack/blob/v3.13/api/messages.proto. I however haven't been successful in building these protos yet.

Current issue is:

go: go.thethings.network/lorawan-stack/v3@v3.12.3 requires
	gopkg.in/DATA-DOG/go-sqlmock.v1@v1.0.0-00010101000000-000000000000: invalid version: unknown revision 000000000000

@jpmeijers
Copy link
Author

I believe I have this working in go. It needs a couple of workaround steps though.

Inside go.mod add

// Depends on go >1.16
go 1.16

// Force a specific version of go-sqlmock as lorawan-stack depends on it, otherwise we get the invalid version error as in my previous comment.
replace gopkg.in/DATA-DOG/go-sqlmock.v1 => gopkg.in/DATA-DOG/go-sqlmock.v1 v1.3.0

require (
...
// We add the lorawan stack as a dependency, to get access to "go.thethings.network/lorawan-stack/v3/pkg/ttnpb"
// TODO: keep this version updated to the latest stack version. Maybe there is a way to automate this?
go.thethings.network/lorawan-stack/v3 v3.12.3
...
}

We need to update the grpc-gateway version.

$ go get github.com/grpc-ecosystem/grpc-gateway/runtime
go get: upgraded github.com/grpc-ecosystem/grpc-gateway v1.14.5 => v1.16.0

After these additions I can do something like this to parse json being POSTed to me with the webhook integration.

	unmarshaler := &jsonpb.Unmarshaler{
		AllowUnknownFields: true, // we don't want to fail on unknown fields
	}

	var packetIn ttnpb.ApplicationUp
	if err := unmarshaler.Unmarshal(strings.NewReader(postbody), &packetIn); err != nil {
		log.Print(err.Error())
	}

TODO: Maybe it is now trivial to switch to protobufs for the webhook integrations to save bandwidth?

@johanstokking
Copy link
Member

	unmarshaler := &jsonpb.Unmarshaler{
		AllowUnknownFields: true, // we don't want to fail on unknown fields
	}

Use go.thethings.network/lorawan-stack/v3/pkg/jsonpb.TTN()

Maybe it is now trivial to switch to protobufs for the webhook integrations to save bandwidth?

Yes. And you can support either by checking the request's Content-Type: application/json for JSONPB vs application/octet-stream for protobuf.


BTW, this issue is about publishing clean protos, that is, .proto files that don't come with options. These protos can be easily compiled, and we'll probably publish the compiled protos for popular languages too. You went through the process of loading The Things Stack as module, which is indeed quite painful because of the replacements and the downgrades. Since The Things Stack is not intended to be used as Go module, we don't provide compatibility on the interfaces. We're getting rid of gogo (#2798) and that will make importing The Things Stack a lot easier though.

@jpmeijers
Copy link
Author

Thanks, that is all very useful recommendations.

Indeed we are getting a bit off-topic here, and the end goal of this issue is indeed to have proto files I can compile to any language. I've tried doing that myself before (TheThingsArchive/api#43) but ran into some dependency hell. Back then I decided to rather use the swagger file to generate my models, which also didn't work correctly. Publishing clean protos (I guess without gogo) which can be easily compile will hopefully make it easier.

I just stumbled upon https://github.com/TheThingsIndustries/docker-protobuf. Is this the same as https://github.com/TheThingsNetwork/api but for TTS v3? In that case, does this repo solve this issue? I guess not really.

What I'd like to have is a way to make sure my API endpoint is always up to date with the protocol the webhook template sends to me. Using a Go module works for now, as I can update the vendors easily to the latest version. Protos that I include and compile each time will however be more generic.

@johanstokking
Copy link
Member

johanstokking commented May 10, 2021

I just stumbled upon TheThingsIndustries/docker-protobuf. Is this the same as TheThingsNetwork/api but for TTS v3? In that case, does this repo solve this issue? I guess not really.

Yes it is, but we are phasing this out as part of moving away from gogo.

We will need a reproducable way to compile protos, and a Docker image is very useful for that, but this particular image is very opinionated and contains lots of magic with proto options.

What I'd like to have is a way to make sure my API endpoint is always up to date with the protocol the webhook template sends to me. Using a Go module works for now, as I can update the vendors easily to the latest version. Protos that I include and compile each time will however be more generic.

Indeed. They will be more generic, but that is also where we are moving with The Things Stack.

In any case, we guarantee that the protos and the JSON stay compatible in all future versions of V3. So if it works now with loading TTS as a module (the hardest way), it will work with we moved away from gogo (easier already because more updated dependencies, less downgrades etc), and also when we publish clean protos that you can compile yourself, and even with the Swagger file that we will also have at some point.

@htdvisser htdvisser added the needs/triage We still need to triage this label Jun 3, 2021
@htdvisser htdvisser removed this from the Backlog milestone Jun 8, 2021
@nejraselimovic nejraselimovic removed the needs/triage We still need to triage this label Jun 11, 2021
@htdvisser htdvisser removed their assignment Jul 20, 2022
@htdvisser htdvisser added the needs/triage We still need to triage this label Jul 20, 2022
@htdvisser
Copy link
Contributor

htdvisser commented Jul 20, 2022

Removing my assignment and bumping back to triage for re-assignment (cc: @NicolasMrad).

Maybe this should be merged into a new "SDK" umbrella issue?

@jpmeijers
Copy link
Author

We are one big step closer to this with #5986

@johanstokking
Copy link
Member

@jpmeijers we made some progress on this again: #6449.

We're now publishing clean protos to Buf: https://buf.build/thethingsnetwork/lorawan-stack. This allows you to generate code using buf generate for many languages, leveraging existing protoc plugins. See https://buf.build/docs/generate/usage/. There is no longer a dependency on this repository needed.

For Go, Node, Java and Swift, Buf also generates them on the fly, e.g. go get buf.build/gen/go/thethingsnetwork/lorawan-stack/protocolbuffers/go@latest and that's it (see https://buf.build/thethingsnetwork/lorawan-stack/assets/main).

This issue can now be closed. We are also planning a SDKs: packages and modules that include generated protos as well as some helpers around allowed field masks per backend component, device creation on IS/NS/AS/JS with rollback, device claiming with rollback, etc. That is outside the scope of this issue.

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
Projects
None yet
Development

No branches or pull requests

6 participants