diff --git a/pkg/search/query.go b/pkg/search/query.go index b1f9001a3..c89b85613 100644 --- a/pkg/search/query.go +++ b/pkg/search/query.go @@ -25,6 +25,7 @@ import ( "log" "net/http" "os" + "reflect" "strconv" "strings" "sync" @@ -570,6 +571,8 @@ type PermanodeConstraint struct { // TODO: // NumClaims *IntConstraint // by owner // Owner blob.Ref // search for permanodes by an owner + + // Note: When adding a field, update hasValueConstraint. } type PermanodeContinueConstraint struct { @@ -963,16 +966,16 @@ func (c *PermanodeConstraint) checkValid() error { return nil } if c.Attr != "" { - if c.NumValue == nil && - c.Value == "" && - c.ValueMatches == nil && - c.ValueInSet == nil { - return errors.New("PermanodeConstraint with Attr requires also setting NumValue, Value, ValueMatches, or ValueInSet") + if c.NumValue == nil && !c.hasValueConstraint() { + return errors.New("PermanodeConstraint with Attr requires also setting NumValue or a value-matching constraint") } if nv := c.NumValue; nv != nil { if nv.ZeroMin { return errors.New("NumValue with ZeroMin makes no sense; matches everything") } + if nv.ZeroMax && c.hasValueConstraint() { + return errors.New("NumValue with ZeroMax makes no sense in conjunction with a value-matching constraint; matches nothing") + } if nv.Min < 0 || nv.Max < 0 { return errors.New("NumValue with negative Min or Max makes no sense") } @@ -981,6 +984,18 @@ func (c *PermanodeConstraint) checkValid() error { return nil } +var numPermanodeFields = reflect.TypeOf(PermanodeConstraint{}).NumField() + +// hasValueConstraint returns true if one or more constraints that check an attribute's value are set. +func (c *PermanodeConstraint) hasValueConstraint() bool { + // If a field has been added or removed, update this after adding the new field to the return statement if necessary. + const expectedFields = 12 + if numPermanodeFields != expectedFields { + panic(fmt.Sprintf("PermanodeConstraint field count changed (now %v rather than %v)", numPermanodeFields, expectedFields)) + } + return c.Value != "" || c.ValueMatches != nil || c.ValueInSet != nil +} + func (c *PermanodeConstraint) blobMatches(s *search, br blob.Ref, bm camtypes.BlobMeta) (ok bool, err error) { if bm.CamliType != "permanode" { return false, nil @@ -1083,21 +1098,23 @@ func (c *PermanodeConstraint) permanodeMatchesAttrVals(s *search, vals []string) if c.NumValue != nil && !c.NumValue.intMatches(int64(len(vals))) { return false, nil } - nmatch := 0 - for _, val := range vals { - match, err := c.permanodeMatchesAttrVal(s, val) - if err != nil { - return false, err + if c.hasValueConstraint() { + nmatch := 0 + for _, val := range vals { + match, err := c.permanodeMatchesAttrVal(s, val) + if err != nil { + return false, err + } + if match { + nmatch++ + } } - if match { - nmatch++ + if nmatch == 0 { + return false, nil + } + if c.ValueAll { + return nmatch == len(vals), nil } - } - if nmatch == 0 { - return false, nil - } - if c.ValueAll { - return nmatch == len(vals), nil } return true, nil } diff --git a/pkg/search/query_test.go b/pkg/search/query_test.go index 611cbd850..38fe785ef 100644 --- a/pkg/search/query_test.go +++ b/pkg/search/query_test.go @@ -424,6 +424,30 @@ func TestQueryPermanodeAttrNumValue(t *testing.T) { }) } +// Tests that NumValue queries with ZeroMax return permanodes without any values. +func TestQueryPermanodeAttrNumValueZeroMax(t *testing.T) { + testQuery(t, func(qt *queryTest) { + id := qt.id + + p1 := id.NewPlannedPermanode("1") + id.AddAttribute(p1, "x", "1") + p2 := id.NewPlannedPermanode("2") + id.AddAttribute(p2, "y", "1") // Permanodes without any attributes are ignored. + + sq := &SearchQuery{ + Constraint: &Constraint{ + Permanode: &PermanodeConstraint{ + Attr: "x", + NumValue: &IntConstraint{ + ZeroMax: true, + }, + }, + }, + } + qt.wantRes(sq, p2) + }) +} + // find a permanode (p2) that has a property being a blobref pointing // to a sub-query func TestQueryPermanodeAttrValueInSet(t *testing.T) {