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

[BUG] Reference nested fields on field type validation causes unexpected error #174

Closed
fenos opened this issue Jan 8, 2025 · 6 comments
Closed
Labels
Bug Something isn't working

Comments

@fenos
Copy link

fenos commented Jan 8, 2025

Description

When i try accessing a nested value from a field level validation using CEL, I get the following error:

compilation error: failed to compile expression coordinates: ERROR: <input>:1:6: unexpected failed resolution of 'google.type.LatLng'\n | (this.latitude < 90 && this.latitude > -90) &&(this.longitude < 180 && this.longitude > -90)\n | .....^\nERROR: <input>:1:28: unexpected failed resolution of 'google.type.LatLng'\n | (this.latitude < 90 && this.latitude > -90) &&(this.longitude < 180 && this.longitude > -90)\n | ...........................^\nERROR: <input>:1:52: unexpected failed resolution of 'google.type.LatLng'\n | (this.latitude < 90 && this.latitude > -90) &&(this.longitude < 180 && this.longitude > -90)\n | ...................................................^\nERROR: <input>:1:76: unexpected failed resolution of 'google.type.LatLng'\n | (this.latitude < 90 && this.latitude > -90) &&(this.longitude < 180 && this.longitude > -90)\n | ...........................................................................^

Steps to Reproduce

syntax = "proto3";

import "google/type/latlng.proto";
import "buf/validate/validate.proto";

message TestMessage {
   google.type.LatLng coordinates = 6 [
      (buf.validate.field).cel = {
      id: 'coordinates',
      expression:
          "(this.latitude < 90 && this.latitude > -90) &&"
          "(this.longitude < 180 && this.longitude > -90)"
    }
  ];
}

Expected Behavior

I can access normally nested values using this.

Actual Behavior

Produces the error mentioned above:

unexpected failed resolution of 'google.type.LatLng

Additional Context

It seems to be working if moving the validation to a message type validation instead of field. But it doesn't seem right to need to have it on the message since it's not accessing any other property outside the field itself

@fenos fenos added the Bug Something isn't working label Jan 8, 2025
@nicksnyder nicksnyder transferred this issue from bufbuild/protovalidate Jan 8, 2025
rodaine added a commit that referenced this issue Jan 8, 2025
@rodaine
Copy link
Member

rodaine commented Jan 8, 2025

Hi, @fenos! I'm sorry you're getting an unexpected error. I attempted to reproduce your issue in https://github.com/bufbuild/protovalidate-go/compare/rodaine/issue-174, but pv-go behaved as you'd expect. This also matches the conformance test cases that cover this as well.

What version of pv-go are you using? If you're using buf for code generation, can you share your buf.yaml and buf.gen.yaml?

@fenos
Copy link
Author

fenos commented Jan 8, 2025

Hey @rodaine thanks a lot for checking this out. It is very strange that it is not working. Could it be because of some import order?

Versions I'm using:

connectrpc.com/validate v0.1.0
github.com/bufbuild/protovalidate-go v0.3.0 // indirect

Here are the bufs files:

buf.yaml

# For details on buf.yaml configuration, visit https://buf.build/docs/configuration/v2/buf-yaml
version: v2
lint:
  use:
    - STANDARD
  except:
    - PACKAGE_DIRECTORY_MATCH
breaking:
  use:
    - FILE
deps:
  - buf.build/googleapis/googleapis:e93e34f48be043dab55be31b4b47f458
  - buf.build/bufbuild/protovalidate

buf.gen.yaml

version: v2
plugins:
#   GO
  - local: protoc-gen-go
    out: gen/go/
    opt: paths=source_relative
  - local: protoc-gen-connect-go
    out: gen/go/
    opt: paths=source_relative

#   JS
  - local: protoc-gen-es
    out: gen/es/
    # Also generate any imported dependencies
    include_imports: true
    # Add more plugin options here
    opt: target=ts
  - local: protoc-gen-connect-query
    out: gen/es/
    opt: target=ts

@fenos
Copy link
Author

fenos commented Jan 8, 2025

@rodaine ah! the issue seemed to be caused by the option opt: paths=source_relative in the protoc-gen-go
not sure the reason why

@rodaine
Copy link
Member

rodaine commented Jan 8, 2025

Using source_relative is usually the right choice. Since you're not using managed mode, you may also want to specify option go_package for your protos as well.

Are you generating the dependencies as well or just your protos? Imports in the generated code for protovalidate should refer to the buf.build/... module and googleapis should use google.golang.org/genproto/...

@fenos
Copy link
Author

fenos commented Jan 8, 2025

@rodaine interesting thanks!
I do have a go_package on my protofile and the generated code includes these dependencies:

// Code generated by protoc-gen-go. DO NOT EDIT.
// versions:
// 	protoc-gen-go v1.36.1
// 	protoc        (unknown)
// source: schema/user/v1/user.proto

package v1

import (
	_ "buf.build/gen/go/bufbuild/protovalidate/protocolbuffers/go/buf/validate"
	date "google.golang.org/genproto/googleapis/type/date"
	latlng "google.golang.org/genproto/googleapis/type/latlng"
	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
	reflect "reflect"
	sync "sync"
)

However, if I add back the option paths=source_relative for some reason it produces the error mention on this issue.
Not entirely sure on the internals

Actually, the issue doesn't seem related to the source path. I tried adding it and removing it but doesn't make any difference.
Not sure why it worked when I removed it in my previous trial. Something that I don't fully understand on the proto resolution is not working as expected. Any help would be appreciated

@fenos
Copy link
Author

fenos commented Jan 8, 2025

Mmmh it turned out i was using an old version of protovalidate-go (v.0.3.0) no idea how it got pulled this old version of protovalidate-go.

As soon as I updated to latest it all works consistently 👍

@fenos fenos closed this as completed Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants