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

Enum validation fails when enum defined in sibling package with same name #387

Closed
codyaray opened this issue Aug 31, 2020 · 6 comments
Closed
Labels
Good First Issue Newcomer-friendly issue Help Wanted Community support requested

Comments

@codyaray
Copy link
Contributor

codyaray commented Aug 31, 2020

I just saw something that looks similar to #321, but in golang and with an incorrect package prefix rather than no prefix. This seems different enough to open a new issue, but I'm happy to merge if you'd like.

We include an enum from another package and set enum.defined_only constraint on it.

kafka.core.v1.MarketplacePartner partner = 2 [(validate.rules).enum.defined_only = true];

But the generated code looks like this:

	if _, ok := v1.MarketplacePartner_name[int32(m.GetPartner())]; m.maskHas(mask, "partner") && !ok {
		return MarketplaceEventValidationError{
			field:  "Partner",
			reason: "value must be one of the defined enum values",
		}
	}

Note that it says v1 instead of an import alias for kafka.core.v1. There's no import for kafka/core/v1 in the .pb.validate.go, and v1 is invalid since its not imported either.

More Details

This could have something to do with how we structure our proto files.

We use a proto monorepo which includes files with a layout like this

kafka
├── core
│   ├── go.mod
│   ├── go.sum
│   └── v1
│       ├── core.pb.go
│       ├── core.pb.validate.go
│       ├── core.proto
│       └── error.go
└── marketplace
       ├── go.mod
       ├── go.sum
       └── v1
           ├── marketplace.pb.go
           ├── marketplace.pb.validate.go
           └── marketplace.proto

Each proto package is nested in a v1, v2, etc directory and go_path is declared in the format like github.com/example/some/package/v1;v1.

The file kafka/marketplace/v1/marketplace.proto includes

syntax = "proto3";

import "kafka/core/v1/core.proto";
import "validate/validate.proto";

package kafka.marketplace.v1; 

option go_package = "github.com/confluentinc/cc-structs/kafka/marketplace/v1;v1";

message MarketplaceEvent {
  string id = 1; // uuid
  kafka.core.v1.MarketplacePartner partner = 2 [(validate.rules).enum.defined_only = true]; // GCP vs. AZURE vs. AWS ...
}

This references the file kafka/core/v1/core.proto, which includes

syntax = "proto3";

package kafka.core.v1;

import "validate/validate.proto";

option go_package = "github.com/confluentinc/cc-structs/kafka/core/v1;v1";

enum MarketplacePartner {
  UNKNOWN = 0;
  GCP = 1;
  AWS = 2;
  AZURE = 3;
}

The generated marketplace.pb.validate.go includes

// Code generated by protoc-gen-validate. DO NOT EDIT.
// source: kafka/marketplace/v1/marketplace.proto

package v1

import (
	"bytes"
	"errors"
	"fmt"
	"net"
	"net/mail"
	"net/url"
	"regexp"
	"strings"
	"time"
	"unicode/utf8"

	"github.com/golang/protobuf/ptypes"
	"github.com/iancoleman/strcase"
	"google.golang.org/genproto/protobuf/field_mask"
)

// ensure the imports are used
var (
	_ = bytes.MinRead
	_ = errors.New("")
	_ = fmt.Print
	_ = utf8.UTFMax
	_ = (*regexp.Regexp)(nil)
	_ = (*strings.Reader)(nil)
	_ = net.IPv4len
	_ = time.Duration(0)
	_ = (*url.URL)(nil)
	_ = (*mail.Address)(nil)
	_ = ptypes.DynamicAny{}
	_ = field_mask.FieldMask{}
	_ = strcase.ToSnake
)

// define the regex for a UUID once up-front
var _marketplace_uuidPattern = regexp.MustCompile("^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$")

// Validate checks the field values on MarketplaceEvent with the rules defined
// in the proto definition for this message. If any rules are violated, an
// error is returned.
func (m *MarketplaceEvent) Validate() error {
	return m.ValidateWithMask(nil)
}

func (m *MarketplaceEvent) ValidateWithMask(mask *field_mask.FieldMask) error {
	if m == nil {
		return nil
	}

	// no validation rules for Id

	if _, ok := v1.MarketplacePartner_name[int32(m.GetPartner())]; m.maskHas(mask, "partner") && !ok {
		return MarketplaceEventValidationError{
			field:  "Partner",
			reason: "value must be one of the defined enum values",
		}
	}
       ...
}

Note the fieldmask/strcase imports come from #366 but they shouldn't have anything to do with this issue.

We can see in the generated .pb.validate.go that we don't have any external enum imports at all.

@stale
Copy link

stale bot commented Oct 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Activity has stalled on this issue/pull-request label Oct 4, 2020
@codyaray
Copy link
Contributor Author

codyaray commented Oct 5, 2020

Probably would be good if this wasn't marked stale just because no one has had time to look at it at all yet. :)

@stale stale bot removed the Stale Activity has stalled on this issue/pull-request label Oct 5, 2020
@stale
Copy link

stale bot commented Nov 16, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Activity has stalled on this issue/pull-request label Nov 16, 2020
@stale stale bot closed this as completed Dec 12, 2020
@codyaray
Copy link
Contributor Author

Hiya - I think this is still an issue

@akonradi akonradi reopened this Jun 25, 2021
@stale stale bot removed the Stale Activity has stalled on this issue/pull-request label Jun 25, 2021
@akonradi akonradi added Good First Issue Newcomer-friendly issue Help Wanted Community support requested labels Jun 25, 2021
@hurrycane
Copy link

I can confirm this is an issue -- no imports for enums defined in a different package.

@mashail
Copy link

mashail commented Apr 27, 2022

I can confirm it too #585

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Newcomer-friendly issue Help Wanted Community support requested
Projects
None yet
Development

No branches or pull requests

5 participants