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.

Sign up for GitHub

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

Jump to bottom

Proto/Swagger HTTP JSON to RPC mapping #13

Open
psprings opened this issue Jul 17, 2019 · 4 comments · May be fixed by #15
Open

Proto/Swagger HTTP JSON to RPC mapping #13

psprings opened this issue Jul 17, 2019 · 4 comments · May be fixed by #15

Comments

@psprings
Copy link

Problem

The current HTTP JSON to RPC mapping does not seem to generate proper string replacement logic, so the HTTP request will always result in a 404.

Example

The following would yield a 404

grafeasCfg := &grafeasAPI.Configuration{
	BasePath:      "http://localhost:8080",
	DefaultHeader: make(map[string]string),
	UserAgent:     "Swagger-Codegen/0.1.0/go",
}
grafeasClient := grafeasAPI.NewAPIClient(grafeasCfg)
projectID := "projects/myproject"
notesResp, _, err := grafeasClient.GrafeasV1Beta1Api.ListNotes(context.Background(), projectID, nil)
if err != nil {
	log.Fatal(err)
} else {
	log.Print(notesResp.Notes)
}

While a curl against that same project would yield a 200

$ curl http://localhost:8080/v1beta1/projects/myproject/notes
{"notes":[],"nextPageToken":""}

Proto generated client

The following shows the broken string replacement logic

// create path and map variables
localVarPath := a.client.cfg.BasePath + "/v1beta1/{parent=projects/*}/notes"
localVarPath = strings.Replace(localVarPath, "{"+"parent"+"}", fmt.Sprintf("%v", parent), -1)

Reference to code

Derived from:
https://github.com/grafeas/grafeas/blob/6a8d995912a9f10f732e8ffcffbae8830507ed17/proto/v1beta1/grafeas.proto#L121

Simulated

parent := "projects/myproject"
localVarPath := "http://localhost:8080" + "/v1beta1/{parent=projects/*}/notes"
fmt.Println("[CURRENT]  BEFORE REPLACEMENT:", localVarPath)
localVarPath = strings.Replace(localVarPath, "{"+"parent"+"}", fmt.Sprintf("%v", parent), -1)
fmt.Println("[CURRENT]  AFTER REPLACEMENT :", localVarPath)
localVarPath = "http://localhost:8080" + "/v1beta1/{parent}/notes"
fmt.Println("[PROPOSED] BEFORE REPLACEMENT:", localVarPath)
localVarPath = strings.Replace(localVarPath, "{"+"parent"+"}", fmt.Sprintf("%v", parent), -1)
fmt.Println("[PROPOSED] AFTER REPLACEMENT :", localVarPath)
// [CURRENT]  BEFORE REPLACEMENT: http://localhost:8080/v1beta1/{parent=projects/*}/notes
// [CURRENT]  AFTER REPLACEMENT : http://localhost:8080/v1beta1/{parent=projects/*}/notes
// [PROPOSED] BEFORE REPLACEMENT: http://localhost:8080/v1beta1/{parent}/notes
// [PROPOSED] AFTER REPLACEMENT : http://localhost:8080/v1beta1/projects/myproject/notes

Solution

I haven't had time to test yet but changing

rpc ListNotes(ListNotesRequest) returns (ListNotesResponse) {
    option (google.api.http) = {
      get: "/v1beta1/{parent=projects/*}/notes"
    };
  };

to

rpc ListNotes(ListNotesRequest) returns (ListNotesResponse) {
    option (google.api.http) = {
      get: "/v1beta1/{parent}/notes"
    };
  };

should work
@aysylu
Copy link
Contributor

aysylu commented Sep 24, 2019

Hi @psprings, thanks so much for filing this issue and for the fix proposal! The underlying issue here is that OpenAPI used by Swagger and GoogleAPI are structured differently. There has been a lot of work to address these issues, but it's still WIP. I think the best way to address this specific issue is to modify the generated library code. Would you be willing to contribute this fix?

@psprings
Copy link
Author

psprings commented Oct 7, 2019

@aysylu sorry, didn't see the response until late. I'd be happy to PR a fix!

@aysylu
Copy link
Contributor

aysylu commented Oct 7, 2019

@psprings no problem! Looking forward to your PR.:)

@psprings psprings linked a pull request Nov 5, 2019 that will close this issue
Open
@psprings
Copy link
Author

psprings commented Nov 5, 2019

@aysylu things have been a little busy, but I had a little bit of time the last night or two to dig into this a little bit.

I threw together an idea for how to modify the spec file(s) as part of the go generate in #15 whenever you have a chance to take a look

I'd be interested in discussing generated folder structure, the inclusion/exclusion of the project.swagger.json file, and if this would make sense to integrate into an upstream CI process (in which case the actual "generation" of the client would occur whenever a tag is created) here or on the PR.

@psprings psprings mentioned this issue Nov 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Initial Swagger compatibility proposal psprings/client-go
2 participants
@aysylu @psprings