chrusty / protoc-gen-jsonschema

Protobuf to JSON-Schema compiler
Apache License 2.0
496 stars 101 forks source link

Enums are not generated when file contains messages #41

Closed tomarrell closed 4 years ago

tomarrell commented 4 years ago

When a file has both enum and message type definitions, the converter does not create files for the enum definitions.

The converter outputs a message:

WARN[0000] protoc-gen-jsonschema will create multiple ENUM schemas from one proto file  proto_filename=shared.proto schemas=2

However, when looking at the files written, there are no corresponding files for the enums.

Having a brief look at the converter, it seems like this is due to the conditional here, https://github.com/chrusty/protoc-gen-jsonschema/blob/master/internal/converter/converter.go#L143, which checks if the number of MessageTypes is zero and generates the enum types. Otherwise, it generates the message types.

Expected functionality would be that both the message and enum types in a file have corresponding output schema files.

Happy to provide more info, and possibly a MRE if needed.

Cheers!

felixjung commented 4 years ago

This bug seems to be hidden by a faulty test. The Enumception test case imports the ImportedEnum. As you can see in the EnumCeption const, the expected schema for ImportedEnum is not correct in the test.

Input proto

// ImportedEnum.proto
syntax = "proto3";
package samples;

enum ImportedEnum {
    VALUE_0 = 0;
    VALUE_1 = 1;
    VALUE_2 = 2;
    VALUE_3 = 3;
}

// Enumception.proto
syntax = "proto3";
package samples;

import "PayloadMessage.proto";
import "ImportedEnum.proto";

message Enumception {
    enum FailureModes {
        RECURSION_ERROR = 0;
        SYNTAX_ERROR    = 1;
    }

    string name                      = 1;
    string timestamp                 = 2;
    int32 id                         = 3;
    float rating                     = 4;
    bool complete                    = 5;
    FailureModes failureMode         = 6;
    PayloadMessage payload           = 7;
    repeated PayloadMessage payloads = 8;
    ImportedEnum importedEnum        = 9;
}

Incorrect Expected JSON Schema

        "importedEnum": {
            "oneOf": [
                {
                    "type": "string"
                },
                {
                    "type": "integer"
                }
            ]
        }

Correct Expected JSON Schema

        "importedEnum": {
            "enum": [
                "VALUE_0",
                0,
                "VALUE_1",
                1,
                "VALUE_2",
                2,
                "VALUE_3",
                3,
            ],
            "oneOf": [
                {
                    "type": "string"
                },
                {
                    "type": "integer"
                }
            ]
        }
chrusty commented 4 years ago

Thanks for reporting this @tomarrell and @felixjung. I'll take a look, see if I can quickly fix it and roll a new version.

felixjung commented 4 years ago

Hey, thanks for the response. We have fixed the issue and will open a PR with additional tests, soon (working on some last kinks right now).

chrusty commented 4 years ago

When a file has both enum and message type definitions, the converter does not create files for the enum definitions.

The converter outputs a message:

WARN[0000] protoc-gen-jsonschema will create multiple ENUM schemas from one proto file  proto_filename=shared.proto schemas=2

However, when looking at the files written, there are no corresponding files for the enums.

Having a brief look at the converter, it seems like this is due to the conditional here, https://github.com/chrusty/protoc-gen-jsonschema/blob/master/internal/converter/converter.go#L143, which checks if the number of MessageTypes is zero and generates the enum types. Otherwise, it generates the message types.

Expected functionality would be that both the message and enum types in a file have corresponding output schema files.

Happy to provide more info, and possibly a MRE if needed.

Cheers!

Hey @tomarrell, I've been testing it out and I think there are two separate issues here.

First, I tried to generate schemas just for the EnumCeption proto: protoc --jsonschema_out=. --proto_path=internal/converter/testdata/proto internal/converter/testdata/proto/Enumception.proto, and you're right - this only made one file. But if I ran it against both protos then I did get two schema files:

protoc --jsonschema_out=. --proto_path=internal/converter/testdata/proto internal/converter/testdata/proto/Enumception.proto internal/converter/testdata/proto/ImportedEnum.proto

Are you able to give that a try and let me know if that solves your problem?

chrusty commented 4 years ago

Hey, thanks for the response. We have fixed the issue and will open a PR with additional tests, soon (working on some last kinks right now).

OK @felixjung, I'd really like to see what you've found. PR is welcome!

felixjung commented 4 years ago

Are you able to give that a try and let me know if that solves your problem?

Yes, this works in as far as that the additional file is generated. However, the importedEnum field inside the message JSON schema still misses the enum values (i.e., the "enum" field of the schema object). It only includes the "oneOf" field.

We''ve tracked the issue down to the proto ProtoPackage struct not having a global register of standalone enums and there not being a registerEnum method similar to the existing registerType method. The PR will introduce this together with lookup methods for enums.

chrusty commented 4 years ago

Thanks @felixjung. I think I'm looking at the same part of the code as you right about now then, and realising that the enum descriptor for this imported enum doesn't contain any values. If you're close then I'll just shutdown for now and see what you can come up with.

Thanks again for your efforts!

chrusty commented 4 years ago

Thanks for your PR @felixjung!