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

If fq_schema_naming is true, use service fullname as the operation tagname #369

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

comynli
Copy link

@comynli comynli commented Nov 15, 2022

When fq_ schema_naming is set to true, use service full name as tag of operation. like schema use message full name as title.

@comynli comynli requested a review from a team as a code owner November 15, 2022 11:27
@google-cla
Copy link

google-cla bot commented Nov 15, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@timburks
Copy link
Contributor

Let's fix the broken tests before merging. I'll take a look later today, we may just need to update the expected results.

@timburks timburks self-requested a review March 25, 2023 20:21
Copy link
Contributor

@timburks timburks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change to the tests:

@@ -163,8 +163,10 @@ func TestOpenAPIFQSchemaNaming(t *testing.T) {
                                }
                        } else {
                                // Verify that the generated spec matches our expected version.
-                               err = exec.Command("diff", TEMP_FILE, fixture).Run()
+                               cmd := exec.Command("diff", TEMP_FILE, fixture)
+                               out, err := cmd.CombinedOutput()
                                if err != nil {
+                                       t.Logf("%s", string(out))
                                        t.Fatalf("Diff failed: %+v", err)
                                }

I see these diffs, which detail the test failures:

go test ./cmd/protoc-gen-openapi
--- FAIL: TestOpenAPIFQSchemaNaming (0.21s)
    --- FAIL: TestOpenAPIFQSchemaNaming/Google_Library_example (0.03s)
        plugin_test.go:169: 6c6
            <     title: google.example.library.v1.LibraryService API
            ---
            >     title: LibraryService API
            549c549
            <     - name: google.example.library.v1.LibraryService
            ---
            >     - name: LibraryService
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Body_mapping (0.02s)
        plugin_test.go:169: 6c6
            <     title: tests.bodymappying.message.v1.Messaging API
            ---
            >     title: Messaging API
            73c73
            <     - name: tests.bodymappying.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Map_fields (0.02s)
        plugin_test.go:169: 6c6
            <     title: tests.mapfields.message.v1.Messaging API
            ---
            >     title: Messaging API
            113c113
            <     - name: tests.mapfields.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Path_params (0.01s)
        plugin_test.go:169: 6c6
            <     title: tests.pathparams.message.v1.Messaging API
            ---
            >     title: Messaging API
            133c133
            <     - name: tests.pathparams.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Protobuf_types (0.02s)
        plugin_test.go:169: 6c6
            <     title: tests.protobuftypes.message.v1.Messaging API
            ---
            >     title: Messaging API
            502c502
            <     - name: tests.protobuftypes.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/RPC_types (0.02s)
        plugin_test.go:169: 6c6
            <     title: tests.rpctypes.message.v1.Status API
            ---
            >     title: Status API
            54c54
            <     - name: tests.rpctypes.message.v1.Status
            ---
            >     - name: Status
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/JSON_options (0.01s)
        plugin_test.go:169: 6c6
            <     title: tests.jsonnames.message.v1.Messaging API
            ---
            >     title: Messaging API
            123c123
            <     - name: tests.jsonnames.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Ignore_services_without_annotations (0.01s)
        plugin_test.go:169: 6c6
            <     title: tests.noannotations.message.v1.Messaging1 API
            ---
            >     title: Messaging1 API
            73c73
            <     - name: tests.noannotations.message.v1.Messaging1
            ---
            >     - name: Messaging1
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/Enum_Options (0.01s)
        plugin_test.go:169: 6c6
            <     title: tests.enumoptions.message.v1.Messaging API
            ---
            >     title: Messaging API
            76c76
            <     - name: tests.enumoptions.message.v1.Messaging
            ---
            >     - name: Messaging
        plugin_test.go:170: Diff failed: exit status 1
    --- FAIL: TestOpenAPIFQSchemaNaming/OpenAPIv3_Annotations (0.02s)
        plugin_test.go:169: 90c90
            <     - name: tests.openapiv3annotations.message.v1.Messaging1
            ---
            >     - name: Messaging1
        plugin_test.go:170: Diff failed: exit status 1
FAIL
FAIL	github.com/google/gnostic/cmd/protoc-gen-openapi	0.950s
FAIL

This is showing changes to the top-level tags array and indirectly, the info.title field. Was that the intent of this PR? The title refers to operation tagname which isn't being set or modified in the tests.

I'm not sure that this is the intended change or if other users would prefer this change, so I'll hold and wait for clarification. Thanks for using this!

@jeffsawatzky
Copy link
Contributor

You can also temporarily set this to true
https://github.com/google/gnostic/blob/main/cmd/protoc-gen-openapi/plugin_test.go#L50
then run the tests. It will recreate the fixtures, which you can then diff to see if the changes make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants