Help testing the next LoRa (App) Server release with LoRaWAN 1.1 support


#21

I think this is related to the same issue :slight_smile: Service-profile listing should be per-organization, not per system. E.g. in my case the request looks like:

https://test.loraserver.io/api/service-profiles?limit=10&organizationID=1

As with your re-generated API, organization_id=1 instead of organizationID=1 is expected, it is considered as 0 thus the filter is not applied. Therefore you see the full list. You will probably see many more issues related to this.

(the background behind this issue is that https://developers.google.com/protocol-buffers/docs/proto3#json defines that fields are lowerCamelCase and can be overridden using the json_name field option in .proto file, which was incorrectly implemented by the gRPC gateway as it was using .getName() instead of .getJsonName()).


#22

Ah, great, I’ll wait for you to fork and include the grpc-gateway then and omit any more of these for now.
Thanks!


#23

If you push the latest master, run make requirements then make api should generate the correct code :slight_smile:


#24

Thanks! Though I’m having a hard time getting it to work.

If I just run make requirements, dep tells me that the fork is unused and make api renders the same old results.

If i change "github.com/grpc-ecosystem/grpc-gateway/runtime" import at root_run.go to point to "github.com/brocaar/grpc-gateway/runtime", then make complains with:

vendor/github.com/brocaar/grpc-gateway/runtime/handler.go:12:2: use of internal package not allowed

Maybe I’m missing something, possibly just a coffee to see my mistake, but I thought I’d ask anyway. :sweat_smile:


#25

Not sure what is going wrong. No need to change the imports. What you need to run is:

go get -u github.com/brocaar/grpc-gateway/protoc-gen-grpc-gateway
go get -u github.com/brocaar/grpc-gateway/protoc-gen-swagger

This is only for generating code so you need to make sure that the protoc-gen-grpc-gateway and protoc-gen-swagger binaries are updated.

Btw, I fixed the error on organization delete!


#26

It’s really odd, I had tried just go get -u ... but it wasn’t working either. I also tried cleaning things and getting them again:

go clean -i github.com/grpc-ecosystem/grpc-gateway/protoc-gen-grpc-gateway...
go clean -i github.com/grpc-ecosystem/grpc-gateway/protoc-gen-swagger...
go clean -i github.com/brocaar/grpc-gateway/protoc-gen-swagger...
go clean -i github.com/brocaar/grpc-gateway/protoc-gen-swagger...
go get -u github.com/brocaar/grpc-gateway/protoc-gen-grpc-gateway
go get -u github.com/brocaar/grpc-gateway/protoc-gen-swagger

Everything remained the same. I then changed gen.sh with GOSUBPATHS="/src:/src/github.com/brocaar/grpc-gateway/third_party/googleapis", but again, no luck.

A couple of times I wiped out every *.pb.go, *.pb.gw.go and *.swagger.json file under api/ before generating them without luck.


#27

You could try which protoc-gen-swagger and which protoc-gen-grpc-gateway to find out where the protoc-gen-... binaries are located and remove these. Or try again using the provided development Docker container?


#28

They where installed at my gopath/bin, I removed both, got your forked versions which installed to the same path but still the swagger.json files are being generated with snake case fields. I’m starting to think there’s some packages cache going on or something like that (though I reran the clean commands with the -cache flag, but still no luck).

I’ll let you know if I fix it.


#29

:man_facepalming: my mistake, I did push my own fork, but as I made the change under $GOPATH/src/github.com/grpc-ecosystem/... it works for me. When you do a go get, it still sees the original code as a requirement as go get installs it under github.com/brocaar/grpc-gateway/..., which then relies on github.com/grpc-ecosystem/grpc-gateway/...

I’m not sure what the best way is, I could update all the import paths in my fork, but that makes it really hard to stay up-to-date with the upstream. Best would be if the GetJsonName change is merged asap into the original repo.


#30

Ah, that was it. Thanks for catching that.

I agree it’d be messy and it’s probably better that those of us developing on top of the source do our own forks of grpc-gateway for the meantime, and then just go back to the original repo when it gets merged. We are all introducing custom stuff and have to keep up with your source anyway.

Edit: Worked like a charm!


#31

In a somewhat related note and to continue with the feedback, when navigating as an admin, changing the selected organization will always navigate to the applications list of the organization instead of the section in which one was on the previous organization. In other words, if I wanted to check all service profiles for some reason, it’s not easy as I need to select the first organization, then select service profiles; select the second organization, then select service profiles; and so on. On the other hand, always going to the applications list is consistent, makes the code simpler and is only an “issue” when entering as an admin, so no burden on the users.

Thought I’d comment on it to see what others think.


#32

The problem here is that one might be an admin in one organization, but not the other. Also you can configure an organization to have gateways while the other can’t have gateways. So a consistent solution would not be possible and the easiest solution is to redirect to the applications page as this is available to any type of user :slight_smile:


#33

Yeah, that’s what I thought.

A couple more things:

When I check a device profile, it correctly fetches everything, but lorawan mac version and regional parameters don’t appear in the form, instead the placeholders appear (though they are correctly set and work fine, is just a visual thing).

Where did the custom encode/decode JS function forms go?


#34

Hmmmm… I thought I fixed the sidebar organization selector, but I broke the autocomplete element for static selects. Thanks, will fix!

The custom encode/decode JS is still under application configuration. I see that the payload form label is missing for that field. Will add!


#35

Right, just missed them.


#36

I’ve updated the form labels. This should be more clear now :slight_smile: (master branch)


#37

One more little bug: when trying to create a device profile as an organization admin, selecting a network server will kick you out because lack of authorization to Get a network server. The quick fix is changing ValidateNetworkServerAccess from this:

	switch flag {
	case Read, Update, Delete:
		// global admin
		where = [][]string{
			{"u.username = $1", "u.is_active = true", "u.is_admin = true"},
		}
	}

To this:

	switch flag {
	case Read:
		//Any active user.
		where = [][]string{
			{"u.username = $1", "u.is_active = true"},
		}
	case Update, Delete:
		// global admin
		where = [][]string{
			{"u.username = $1", "u.is_active = true", "u.is_admin = true"},
		}
	}

Of course, you may want to refine this to allow only organization admins to get the network server.


#38

Thanks, I’ll investigate this!


#39

This issue has been fixed :slight_smile:


#40

One more little thing: when trying to login you need to do it twice because you aren’t preventing the form from submitting. This fixs it for me:

  onSubmit(e) {
    e.preventDefault();
    SessionStore.login(this.state.login, () => {
      this.props.history.push("/");
    });
  }