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

Add a way to say "all fields" in the projection #155

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions schema/query/projection_evaluator.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,27 @@ func (p Projection) Eval(ctx context.Context, payload map[string]interface{}, rs
return payload, err
}

func checkStar(p Projection) (bool, error) {
hasStar := false
s := ""
for i := range p {
Copy link
Collaborator

@smyrman smyrman Oct 17, 2017

Choose a reason for hiding this comment

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

This for loop is a bit awkward...

Could not this whole test be just:

func checkStar(p Projection) (bool, error) {
    if len(p) == 1 && p[0].Name == "*" && len p[0].Children == 0 {
        return true, nil
    }
    if len(p) <= 1 {
        return false, nil
    }
    for i := range p {
        if p[i].Name == "*"  && len p[i].Children == 0: {
            return false, fmt.Errorf("%s: invalid value: * must be the only field", p)
        }
    }
    return false, nil
}

This may give a slightly more clear LOS (Line of Sight).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Otherwise I am now OK with * only being allowed as the only parameter, as long as this now expands all references; at least initially. May place nice (end-user-wise) with the suggesrion of using * to expand references in schema.Dict and schema.Array

#138

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions @smyrman. However expanding referenced fields, is what I wanted to avoid in the first place. My indention was to provide extension to this requests:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55

Which will return only non referenced fields, so making this request:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*,visits

Is to actually add the visits referenced resource, without mentioning all the non referenced fields.

Initially I though this as a low hanging fruit, but it gets more complicated. It is not big deal to enumerate all the fields explicitly, so I think I will just abandon this PR.

Copy link
Collaborator

@smyrman smyrman Oct 18, 2017

Choose a reason for hiding this comment

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

It is not big deal to enumerate all the fields explicitly, so I think I will just abandon this PR.

Alright 👍 Might be wise for now. This is also the way we use it.

Just for my clarification though.

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*,visits

Is to actually add the visits referenced resource, without mentioning all the non referenced fields.

This is also what I though was the initial intent. However, my (perhaps wrongful) understanding of the final PR was that it would expand "visits", all other references and maybe (not clear to me) all fields that are hidden by default (Field.Hidden == true) if you did this:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*

If we rather want the initially described behaviour, I suppose the following might work:

// expandStar replace a single `*` in projection with all fields at the position of `*`.
// duplicated fields will be handled according to the last-win principal.
expandStart(p *Projection, schema.Fields) {
    ...
}

And then no other changes. This will also ensure errors later if *is part of a field-name to be as before without the extra checks added in this PR, and withourt accepting * in the legal field character list.

Anyway, still maybe best to leave it for now, and consider it a bit closer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=*

The request above would have been equivalent to this:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55

No referenced fields, no hidden fields.
Actually there is no way of getting the hidden fields with the current code. Mentioning them explicitly in the fields= parameter results in the following error:

GET http://192.168.0.103:3001/api/clients/59a85126eaba9ebd0180ac55?fields=nameTokens
"Invalid `fields` parameter: nameTokens: hidden field"
Client = schema.Schema{
		Fields: schema.Fields{
			"id": mongo.ObjectIDField,
...
			"nameTokens": {
				Hidden: true,
			},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Closing the PR now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your effort, even if it was not merged this time.

if i > 0 {
s += ","
}
s += p[i].Name

if len(p[i].Children) == 0 {
if p[i].Name == "*" {
hasStar = true
}
if hasStar && i != 0 {
return false, fmt.Errorf("%s: invalid value: * must be the only field", s)
}
}
}
return hasStar, nil
}

func evalProjection(ctx context.Context, p Projection, payload map[string]interface{}, validator schema.Validator, rbr *referenceBatchResolver) (map[string]interface{}, error) {
res := map[string]interface{}{}
resMu := sync.Mutex{} // XXX use sync.Map once Go 1.9 is out
Expand All @@ -50,6 +71,18 @@ func evalProjection(ctx context.Context, p Projection, payload map[string]interf
p = append(p, ProjectionField{Name: fn})
}
}

isStar, err := checkStar(p)
if err != nil {
return nil, err
}
if isStar {
// When projection is star, it's like saying "all fields".
for fn := range payload {
p = append(p, ProjectionField{Name: fn})
}
}

for i := range p {
pf := p[i]
name := pf.Name
Expand Down
56 changes: 56 additions & 0 deletions schema/query/projection_evaluator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,62 @@ func TestProjectionEval(t *testing.T) {
nil,
`{"connection":[{"name":"third"}]}`,
},
{
"ValidStar",
`*`,
`{"parent":{"child":"value"},"simple":"value"}`,
nil,
`{"parent":{"child":"value"},"simple":"value"}`,
},
{
"Double/InvalidStar",
`*,*`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("*,*: invalid value: * must be the only field"),
``,
},
{
"Parent/InvalidStar1",
`*,parent`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("*,parent: invalid value: * must be the only field"),
``,
},
{
"Parent/InvalidStar2",
`parent,*`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("parent,*: invalid value: * must be the only field"),
``,
},
{
"Child/ValidStar",
`parent{*}`,
`{"parent":{"child":"value"},"simple":"value"}`,
nil,
`{"parent":{"child":"value"}}`,
},
{
"Child/InvalidStar1",
`parent{*,child}`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("parent.*,child: invalid value: * must be the only field"),
``,
},
{
"Child/InvalidStar2",
`parent{child,*}`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("parent.child,*: invalid value: * must be the only field"),
``,
},
{
"Parent/Child/InvalidStar",
`parent{child,*},*`,
`{"parent":{"child":"value"},"simple":"value"}`,
errors.New("parent,*: invalid value: * must be the only field"),
``,
},
}

for i := range cases {
Expand Down
7 changes: 6 additions & 1 deletion schema/query/projection_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,12 @@ func (p *projectionParser) scanFieldName() string {
field := []byte{}
for p.more() {
c := p.peek()
if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' {
if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '*' {
Copy link
Owner

Choose a reason for hiding this comment

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

This will allow * in field names, which is not what we want right? The * should only be allowed by itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will fix this.

// Allow * only by itself.
if (c == '*' && len(field) != 0) || (c != '*' && len(field) > 1 && field[0] == '*') {
return ""
}

field = append(field, c)
p.pos++
continue
Expand Down
4 changes: 4 additions & 0 deletions schema/query/projection_validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import (

// Validate validates the projection field against the provided validator.
func (pf ProjectionField) Validate(validator schema.Validator) error {
if pf.Name == "*" && len(pf.Children) == 0 && len(pf.Params) == 0 {
return nil
}

def := validator.GetField(pf.Name)
if def == nil {
return fmt.Errorf("%s: unknown field", pf.Name)
Expand Down
44 changes: 25 additions & 19 deletions schema/query/projection_validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,32 +34,38 @@ func TestProjectionValidate(t *testing.T) {
}
cases := []struct {
projection string
err error
parseErr error
validErr error
}{
{`parent{child},simple`, nil},
{`with_params(foo:1)`, nil},
{`with_params(bar:true)`, nil},
{`with_params(bar:false)`, nil},
{`with_params(foobar:"foobar")`, nil},
{`foo`, errors.New("foo: unknown field")},
{`simple{child}`, errors.New("simple: field as no children")},
{`parent{foo}`, errors.New("parent.foo: unknown field")},
{`simple(foo:1)`, errors.New("simple: params not allowed")},
{`with_params(baz:1)`, errors.New("with_params: unsupported param name: baz")},
{`with_params(foo:"a string")`, errors.New("with_params: invalid param `foo' value: not an integer")},
{`with_params(foo:3.14)`, errors.New("with_params: invalid param `foo' value: not an integer")},
{`with_params(bar:1)`, errors.New("with_params: invalid param `bar' value: not a Boolean")},
{`with_params(foobar:true)`, errors.New("with_params: invalid param `foobar' value: not a string")},
{`parent{child},simple`, nil, nil},
{`with_params(foo:1)`, nil, nil},
{`with_params(bar:true)`, nil, nil},
{`with_params(bar:false)`, nil, nil},
{`with_params(foobar:"foobar")`, nil, nil},
{`foo`, nil, errors.New("foo: unknown field")},
{`simple{child}`, nil, errors.New("simple: field as no children")},
{`parent{foo}`, nil, errors.New("parent.foo: unknown field")},
{`simple(foo:1)`, nil, errors.New("simple: params not allowed")},
{`with_params(baz:1)`, nil, errors.New("with_params: unsupported param name: baz")},
{`with_params(foo:"a string")`, nil, errors.New("with_params: invalid param `foo' value: not an integer")},
{`with_params(foo:3.14)`, nil, errors.New("with_params: invalid param `foo' value: not an integer")},
{`with_params(bar:1)`, nil, errors.New("with_params: invalid param `bar' value: not a Boolean")},
{`with_params(foobar:true)`, nil, errors.New("with_params: invalid param `foobar' value: not a string")},
{`*`, nil, nil},
{`parent,*`, nil, nil},
{`*,parent`, nil, nil},
{`*parent`, errors.New("looking for field name at char 2"), nil},
{`parent*`, errors.New("looking for field name at char 6"), nil},
}
for i := range cases {
tc := cases[i]
t.Run(tc.projection, func(t *testing.T) {
pr, err := ParseProjection(tc.projection)
if err != nil {
t.Errorf("ParseProjection unexpected error: %v", err)
if !reflect.DeepEqual(err, tc.parseErr) {
t.Errorf("ParseProjection error = %v, wanted: %v", err, tc.parseErr)
}
if err = pr.Validate(s); !reflect.DeepEqual(err, tc.err) {
t.Errorf("Projection.Validate error = %v, wanted: %v", err, tc.err)
if err = pr.Validate(s); !reflect.DeepEqual(err, tc.validErr) {
t.Errorf("Projection.Validate error = %v, wanted: %v", err, tc.validErr)
}
})
}
Expand Down