-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
Exposing an interface will not prevent calling methods from the underlying type #744
Comments
Hello! Yes, I think this is a problem. Expr uses reflect to check if method is available on provided instance (not interface). As during runtime real object is present. We need to improve the type checker, and enforce strict validation during compile time. Nice idea with |
I hit this issue as well, but slightly different. In my case, I pass an interface to expr during compile time with Minimal reproducer: package main
import "github.com/expr-lang/expr"
type greeter interface {
Hi() string
}
type say struct {
name string
}
func (s say) Hi() string {
return "Hi " + s.name
}
var _ greeter = say{}
func main() {
var err error
s := say{name: "foo"}
_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(s))
if err != nil {
panic("struct: " + err.Error())
}
var g greeter = say{name: "foo"}
_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(g))
if err != nil {
panic("non nil interface: " + err.Error())
}
var nilGreeter greeter
_, err = expr.Compile(`Hi() == "Hi foo"`, expr.Env(nilGreeter))
if err != nil {
panic("nil interface: " + err.Error())
}
} Expected output: no panic Actual output:
Side note: when I created the above reproducer, I found that expr also is not able to handle types derived from base types, which provide methods. My initial definition for type say string
func (s say) Hi() string {
return "Hi " + string(s)
} |
@breml @StarpTech I fixed the problem with interfaces. Now its possible to "hide" parts of methods with interfaces. package interface_test
import (
"testing"
"github.com/expr-lang/expr"
"github.com/expr-lang/expr/internal/testify/assert"
)
type StoreInterface interface {
Get(string) int
}
type StoreImpt struct{}
func (f StoreImpt) Get(s string) int {
return 42
}
func (f StoreImpt) Set(s string, i int) bool {
return true
}
type Env struct {
Store StoreInterface `expr:"store"`
}
func TestInterfaceHide(t *testing.T) {
var env Env
p, err := expr.Compile(`store.Get("foo")`, expr.Env(env))
assert.NoError(t, err)
out, err := expr.Run(p, Env{Store: StoreImpt{}})
assert.NoError(t, err)
assert.Equal(t, 42, out)
_, err = expr.Compile(`store.Set("foo", 100)`, expr.Env(env))
assert.Error(t, err)
assert.Contains(t, err.Error(), "type interface_test.StoreInterface has no method Set")
} |
@breml about those types, yes. This is another thing which need to be developed. type say string But we need to figure out a correct way to do so. For example, Golang requires explicit conversion. |
@antonmedv Thanks for the fix. Can you elaborate, why it is not possible to "hide" part of the methods?
I created #766 for the issue with the derived types. |
A typo! Now it is possible) |
That's great, thank you! |
Hello, as titled. We figured out that the library doesn't hide methods that aren't accessible by the interface definition. We would expect that interface boundaries are enforced.
Simple example:
Pseudocode:
Workaround
It looks like
expr:"-"
can be used as a tag to prevent exposing fields. I haven't find it in the docs unfortunately.The text was updated successfully, but these errors were encountered: