feat(smartplaylist): let a rule set match any rule, not only all of them
The conditions were joined with " AND " and nothing else, so a smart playlist could only ever narrow: "jazz released after 1960" was expressible and "jazz or blues" was not, which is most of what anyone reaches for a second rule to say. `RuleSet.Match` is "all" or "any", and an empty match is "all" — which is what every playlist saved before the field existed carries, so an upgrade cannot silently widen one. ParseRuleSet rejects anything else rather than falling through to AND, since a playlist quietly returning the wrong tracks is worse than one that refuses to be saved. Under OR each condition is parenthesised and under AND it is not: AND is the tighter operator, so an OR-join has to protect a condition carrying a top-level AND of its own — `days_since_played less_than` is two predicates belonging to one rule. The editor shows the choice as a sentence with the control in the middle, and hides it while there is one rule: with nothing to combine, all and any are the same query. Closes #35 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -28,6 +28,7 @@ var (
|
|||||||
errUnsupportedOp = errors.New("unsupported operator")
|
errUnsupportedOp = errors.New("unsupported operator")
|
||||||
errInvalidSortField = errors.New("invalid sort field: not in allowed field list")
|
errInvalidSortField = errors.New("invalid sort field: not in allowed field list")
|
||||||
errNotNumeric = errors.New("value must be numeric")
|
errNotNumeric = errors.New("value must be numeric")
|
||||||
|
errInvalidMatch = errors.New("match must be \"all\" or \"any\"")
|
||||||
)
|
)
|
||||||
|
|
||||||
// Rule represents a single filter condition for a smart playlist.
|
// Rule represents a single filter condition for a smart playlist.
|
||||||
@@ -37,13 +38,45 @@ type Rule struct {
|
|||||||
Value string `json:"value"`
|
Value string `json:"value"`
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// MatchType decides how a rule set's conditions combine.
|
||||||
|
//
|
||||||
|
// The rules used to be joined with " AND " and nothing else, so a
|
||||||
|
// playlist could only ever narrow: "jazz released after 1960" was
|
||||||
|
// expressible and "jazz or blues" was not, which is most of what
|
||||||
|
// anyone reaches for a second rule to say.
|
||||||
|
type MatchType string
|
||||||
|
|
||||||
|
const (
|
||||||
|
// MatchAll requires every rule to hold — the historical behaviour,
|
||||||
|
// and what an empty match means so that every rule set written
|
||||||
|
// before this existed keeps the meaning it was saved with.
|
||||||
|
MatchAll MatchType = "all"
|
||||||
|
// MatchAny requires at least one rule to hold.
|
||||||
|
MatchAny MatchType = "any"
|
||||||
|
)
|
||||||
|
|
||||||
|
// joiner returns the SQL keyword that combines two conditions.
|
||||||
|
// An unrecognised value cannot reach here — ParseRuleSet rejects one
|
||||||
|
// — so the default is about the empty string, which is every rule set
|
||||||
|
// saved before this field existed.
|
||||||
|
func (m MatchType) joiner() string {
|
||||||
|
if m == MatchAny {
|
||||||
|
return " OR "
|
||||||
|
}
|
||||||
|
|
||||||
|
return " AND "
|
||||||
|
}
|
||||||
|
|
||||||
// RuleSet holds the complete filter configuration for a smart
|
// RuleSet holds the complete filter configuration for a smart
|
||||||
// playlist, including optional sort and limit.
|
// playlist, including optional sort and limit.
|
||||||
type RuleSet struct {
|
type RuleSet struct {
|
||||||
Rules []Rule `json:"rules"`
|
Rules []Rule `json:"rules"`
|
||||||
Limit int `json:"limit,omitempty"`
|
// Match is "all" or "any"; empty means "all". It is omitempty so
|
||||||
SortField string `json:"sort_field,omitempty"`
|
// an untouched playlist's stored JSON does not change shape.
|
||||||
SortDir string `json:"sort_dir,omitempty"`
|
Match MatchType `json:"match,omitempty"`
|
||||||
|
Limit int `json:"limit,omitempty"`
|
||||||
|
SortField string `json:"sort_field,omitempty"`
|
||||||
|
SortDir string `json:"sort_dir,omitempty"`
|
||||||
}
|
}
|
||||||
|
|
||||||
// fieldMap maps user-facing rule field names to track_metadata column
|
// fieldMap maps user-facing rule field names to track_metadata column
|
||||||
@@ -116,7 +149,12 @@ const genreDelimiter = "||"
|
|||||||
// slice of rules. It is a pure function — no database access needed.
|
// slice of rules. It is a pure function — no database access needed.
|
||||||
// Returns the clause (without the leading "WHERE"), the parameter
|
// Returns the clause (without the leading "WHERE"), the parameter
|
||||||
// args, and any validation error.
|
// args, and any validation error.
|
||||||
func BuildWhereClause(rules []Rule) (string, []any, error) {
|
//
|
||||||
|
// match decides how the conditions combine; an empty match is MatchAll,
|
||||||
|
// which is what every rule set saved before the field existed means.
|
||||||
|
func BuildWhereClause(
|
||||||
|
rules []Rule, match MatchType,
|
||||||
|
) (string, []any, error) {
|
||||||
if len(rules) == 0 {
|
if len(rules) == 0 {
|
||||||
return "", nil, nil
|
return "", nil, nil
|
||||||
}
|
}
|
||||||
@@ -179,7 +217,28 @@ func BuildWhereClause(rules []Rule) (string, []any, error) {
|
|||||||
args = append(args, condArgs...)
|
args = append(args, condArgs...)
|
||||||
}
|
}
|
||||||
|
|
||||||
return strings.Join(conditions, " AND "), args, nil
|
// Under OR, each condition is parenthesised; under AND it is not.
|
||||||
|
//
|
||||||
|
// The asymmetry is deliberate rather than an omission. AND is the
|
||||||
|
// tighter operator in SQL, so an OR-join has to protect any
|
||||||
|
// condition that contains a top-level AND of its own or the halves
|
||||||
|
// come apart: `days_since_played less_than` is
|
||||||
|
// `last_played IS NOT NULL AND <expr> < ?`, which read without
|
||||||
|
// brackets under an OR-join happens to still parse correctly and
|
||||||
|
// would stop doing so the moment a condition grows a top-level OR.
|
||||||
|
// Bracketing under AND would be a no-op semantically and would
|
||||||
|
// rewrite the clause every existing test pins, so the brackets go
|
||||||
|
// exactly where they change something.
|
||||||
|
if match == MatchAny {
|
||||||
|
bracketed := make([]string, len(conditions))
|
||||||
|
for i, cond := range conditions {
|
||||||
|
bracketed[i] = "(" + cond + ")"
|
||||||
|
}
|
||||||
|
|
||||||
|
conditions = bracketed
|
||||||
|
}
|
||||||
|
|
||||||
|
return strings.Join(conditions, match.joiner()), args, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// validateOperator checks that the operator is valid for the field
|
// validateOperator checks that the operator is valid for the field
|
||||||
@@ -599,7 +658,7 @@ func Evaluate(
|
|||||||
start := time.Now()
|
start := time.Now()
|
||||||
logger := db.Logger()
|
logger := db.Logger()
|
||||||
|
|
||||||
where, args, err := BuildWhereClause(ruleSet.Rules)
|
where, args, err := BuildWhereClause(ruleSet.Rules, ruleSet.Match)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, fmt.Errorf(
|
return nil, fmt.Errorf(
|
||||||
"smart playlist rule error: %w", err,
|
"smart playlist rule error: %w", err,
|
||||||
@@ -1036,6 +1095,16 @@ func ParseRuleSet(jsonStr string) (RuleSet, error) {
|
|||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// A match nobody recognises would otherwise fall through to AND,
|
||||||
|
// which is a playlist quietly returning the wrong tracks rather
|
||||||
|
// than refusing to be saved. This is the only place a rule set
|
||||||
|
// enters the backend, so it is the only place that has to ask.
|
||||||
|
if rs.Match != "" && rs.Match != MatchAll && rs.Match != MatchAny {
|
||||||
|
return RuleSet{}, fmt.Errorf(
|
||||||
|
"%w: %q", errInvalidMatch, rs.Match,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
return rs, nil
|
return rs, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package smartplaylist
|
package smartplaylist
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"errors"
|
||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
@@ -170,7 +171,7 @@ func TestBuildWhereClause_TextIs(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "artist", Operator: "is", Value: "Queen"},
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -189,7 +190,7 @@ func TestBuildWhereClause_TextIsNot(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "artist", Operator: "is_not", Value: "Queen"},
|
{Field: "artist", Operator: "is_not", Value: "Queen"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -209,7 +210,7 @@ func TestBuildWhereClause_TextContains(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "title", Operator: "contains", Value: "Black"},
|
{Field: "title", Operator: "contains", Value: "Black"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -231,7 +232,7 @@ func TestBuildWhereClause_TextDoesNotContain(t *testing.T) {
|
|||||||
Field: "title", Operator: "does_not_contain",
|
Field: "title", Operator: "does_not_contain",
|
||||||
Value: "Black",
|
Value: "Black",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -251,7 +252,7 @@ func TestBuildWhereClause_TextStartsWith(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "title", Operator: "starts_with", Value: "Back"},
|
{Field: "title", Operator: "starts_with", Value: "Back"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -270,7 +271,7 @@ func TestBuildWhereClause_TextEndsWith(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "title", Operator: "ends_with", Value: "Black"},
|
{Field: "title", Operator: "ends_with", Value: "Black"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -292,7 +293,7 @@ func TestBuildWhereClause_TextIsAnyOf(t *testing.T) {
|
|||||||
Field: "artist", Operator: "is_any_of",
|
Field: "artist", Operator: "is_any_of",
|
||||||
Value: `["Queen","AC/DC"]`,
|
Value: `["Queen","AC/DC"]`,
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -312,7 +313,7 @@ func TestBuildWhereClause_NumericIs(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "year", Operator: "is", Value: "1980"},
|
{Field: "year", Operator: "is", Value: "1980"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -331,7 +332,7 @@ func TestBuildWhereClause_NumericIsNot(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "year", Operator: "is_not", Value: "1980"},
|
{Field: "year", Operator: "is_not", Value: "1980"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -350,7 +351,7 @@ func TestBuildWhereClause_NumericGreaterThan(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "year", Operator: "greater_than", Value: "2000"},
|
{Field: "year", Operator: "greater_than", Value: "2000"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -369,7 +370,7 @@ func TestBuildWhereClause_NumericLessThan(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "year", Operator: "less_than", Value: "1980"},
|
{Field: "year", Operator: "less_than", Value: "1980"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -391,7 +392,7 @@ func TestBuildWhereClause_NumericBetween(t *testing.T) {
|
|||||||
Field: "year", Operator: "between",
|
Field: "year", Operator: "between",
|
||||||
Value: "1975,1985",
|
Value: "1975,1985",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -414,7 +415,7 @@ func TestBuildWhereClause_NumericBetweenJSON(t *testing.T) {
|
|||||||
Field: "year", Operator: "between",
|
Field: "year", Operator: "between",
|
||||||
Value: `["1975","1985"]`,
|
Value: `["1975","1985"]`,
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -434,7 +435,7 @@ func TestBuildWhereClause_GenreIsProducesSubquery(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "genre", Operator: "is", Value: "Rock"},
|
{Field: "genre", Operator: "is", Value: "Rock"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -466,7 +467,7 @@ func TestBuildWhereClause_GenreIsNotProducesSubquery(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "genre", Operator: "is_not", Value: "Rock"},
|
{Field: "genre", Operator: "is_not", Value: "Rock"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -495,7 +496,7 @@ func TestBuildWhereClause_GenreIsAnyOfProducesSubquery(t *testing.T) {
|
|||||||
Field: "genre", Operator: "is_any_of",
|
Field: "genre", Operator: "is_any_of",
|
||||||
Value: `["Rock","Pop"]`,
|
Value: `["Rock","Pop"]`,
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -524,7 +525,7 @@ func TestBuildWhereClause_GenreContainsUsesSubquery(t *testing.T) {
|
|||||||
|
|
||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "genre", Operator: "contains", Value: "Rock"},
|
{Field: "genre", Operator: "contains", Value: "Rock"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -557,7 +558,7 @@ func TestBuildWhereClause_MultipleRulesAND(t *testing.T) {
|
|||||||
clause, args, err := BuildWhereClause([]Rule{
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
{Field: "artist", Operator: "is", Value: "Queen"},
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
{Field: "year", Operator: "greater_than", Value: "1975"},
|
{Field: "year", Operator: "greater_than", Value: "1975"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -572,6 +573,110 @@ func TestBuildWhereClause_MultipleRulesAND(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestBuildWhereClause_MultipleRulesOR(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
clause, args, err := BuildWhereClause([]Rule{
|
||||||
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
|
{Field: "year", Operator: "greater_than", Value: "1975"},
|
||||||
|
}, MatchAny)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
want := "(artist_name = ? COLLATE NOCASE) OR (year > ?)"
|
||||||
|
if clause != want {
|
||||||
|
t.Errorf("clause = %q, want %q", clause, want)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(args) != 2 || args[0] != "Queen" || args[1] != int64(1975) {
|
||||||
|
t.Errorf("args = %v, want [Queen 1975]", args)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// An empty match is what every rule set saved before the field existed
|
||||||
|
// carries, and it has to keep meaning AND — a playlist silently
|
||||||
|
// widening to OR on upgrade is the whole risk of adding this field.
|
||||||
|
func TestBuildWhereClause_EmptyMatchIsAll(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
rules := []Rule{
|
||||||
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
|
{Field: "year", Operator: "greater_than", Value: "1975"},
|
||||||
|
}
|
||||||
|
|
||||||
|
empty, _, err := BuildWhereClause(rules, "")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
all, _, err := BuildWhereClause(rules, MatchAll)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if empty != all {
|
||||||
|
t.Errorf("empty match = %q, want the same as MatchAll %q",
|
||||||
|
empty, all)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// A condition carrying its own top-level AND is what makes the
|
||||||
|
// bracketing under OR load-bearing: `days_since_played less_than`
|
||||||
|
// is two predicates, and both belong to the same rule.
|
||||||
|
func TestBuildWhereClause_ORBracketsCompoundCondition(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
clause, _, err := BuildWhereClause([]Rule{
|
||||||
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
|
{
|
||||||
|
Field: "days_since_played",
|
||||||
|
Operator: "less_than",
|
||||||
|
Value: "30",
|
||||||
|
},
|
||||||
|
}, MatchAny)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if !strings.Contains(clause, "(last_played IS NOT NULL AND") {
|
||||||
|
t.Errorf(
|
||||||
|
"compound condition is not bracketed under OR: %q",
|
||||||
|
clause,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseRuleSet_RejectsUnknownMatch(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
_, err := ParseRuleSet(`{"rules":[],"match":"either"}`)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected an error for an unknown match type")
|
||||||
|
}
|
||||||
|
|
||||||
|
if !errors.Is(err, errInvalidMatch) {
|
||||||
|
t.Errorf("err = %v, want errInvalidMatch", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestParseRuleSet_AcceptsAnyAndAll(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
for _, want := range []MatchType{MatchAll, MatchAny} {
|
||||||
|
rs, err := ParseRuleSet(
|
||||||
|
`{"rules":[],"match":"` + string(want) + `"}`,
|
||||||
|
)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("match %q: unexpected error: %v", want, err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if rs.Match != want {
|
||||||
|
t.Errorf("match = %q, want %q", rs.Match, want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) {
|
func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
@@ -581,7 +686,7 @@ func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) {
|
|||||||
Field: "genre", Operator: "does_not_contain",
|
Field: "genre", Operator: "does_not_contain",
|
||||||
Value: "Punk",
|
Value: "Punk",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -609,7 +714,7 @@ func TestBuildWhereClause_SameFieldMultipleTimes(t *testing.T) {
|
|||||||
func TestBuildWhereClause_EmptyRules(t *testing.T) {
|
func TestBuildWhereClause_EmptyRules(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
clause, args, err := BuildWhereClause(nil)
|
clause, args, err := BuildWhereClause(nil, MatchAll)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -631,7 +736,7 @@ func TestBuildWhereClause_InvalidField(t *testing.T) {
|
|||||||
Field: "nonexistent", Operator: "is",
|
Field: "nonexistent", Operator: "is",
|
||||||
Value: "anything",
|
Value: "anything",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal("expected error for invalid field, got nil")
|
t.Fatal("expected error for invalid field, got nil")
|
||||||
}
|
}
|
||||||
@@ -654,7 +759,7 @@ func TestBuildWhereClause_InvalidOperatorForNumeric(t *testing.T) {
|
|||||||
|
|
||||||
_, _, err := BuildWhereClause([]Rule{
|
_, _, err := BuildWhereClause([]Rule{
|
||||||
{Field: "year", Operator: "contains", Value: "1980"},
|
{Field: "year", Operator: "contains", Value: "1980"},
|
||||||
})
|
}, MatchAll)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal(
|
t.Fatal(
|
||||||
"expected error for text operator on numeric field",
|
"expected error for text operator on numeric field",
|
||||||
@@ -676,7 +781,7 @@ func TestBuildWhereClause_InvalidOperatorForText(t *testing.T) {
|
|||||||
Field: "artist", Operator: "greater_than",
|
Field: "artist", Operator: "greater_than",
|
||||||
Value: "Queen",
|
Value: "Queen",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal(
|
t.Fatal(
|
||||||
"expected error for numeric operator on text field",
|
"expected error for numeric operator on text field",
|
||||||
@@ -723,6 +828,80 @@ func TestEvaluate_TextIs(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Two rules that share no track at all: under AND this is empty, and
|
||||||
|
// under OR it is the union. Before Match existed only the first was
|
||||||
|
// expressible, so a playlist could only ever narrow — "jazz or blues"
|
||||||
|
// had no way to be said.
|
||||||
|
func TestEvaluate_MatchAnyUnionsWhereMatchAllIntersects(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
db := database.NewTestDB(t)
|
||||||
|
seedSmartPlaylistData(t, db)
|
||||||
|
|
||||||
|
// Queen has two tracks; Beyoncé has one; no track is by both.
|
||||||
|
rules := []Rule{
|
||||||
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
|
{Field: "artist", Operator: "is", Value: "Beyoncé"},
|
||||||
|
}
|
||||||
|
|
||||||
|
all, err := Evaluate(db, RuleSet{Rules: rules, Match: MatchAll})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Evaluate(all): %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(all) != 0 {
|
||||||
|
t.Errorf("match=all returned %d tracks, want 0", len(all))
|
||||||
|
}
|
||||||
|
|
||||||
|
either, err := Evaluate(db, RuleSet{Rules: rules, Match: MatchAny})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Evaluate(any): %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
if len(either) != 3 {
|
||||||
|
t.Fatalf("match=any returned %d tracks, want 3", len(either))
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tr := range either {
|
||||||
|
if tr.ArtistName != "Queen" && tr.ArtistName != "Beyoncé" {
|
||||||
|
t.Errorf(
|
||||||
|
"track %q has artist %q, want Queen or Beyoncé",
|
||||||
|
tr.TrackName, tr.ArtistName,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// An empty match is what every playlist saved before the field existed
|
||||||
|
// carries, and it has to keep meaning AND all the way through Evaluate
|
||||||
|
// — a stored playlist silently widening on upgrade is the only real
|
||||||
|
// risk in adding this.
|
||||||
|
func TestEvaluate_EmptyMatchStillIntersects(t *testing.T) {
|
||||||
|
t.Parallel()
|
||||||
|
|
||||||
|
db := database.NewTestDB(t)
|
||||||
|
seedSmartPlaylistData(t, db)
|
||||||
|
|
||||||
|
tracks, err := Evaluate(db, RuleSet{
|
||||||
|
Rules: []Rule{
|
||||||
|
{Field: "artist", Operator: "is", Value: "Queen"},
|
||||||
|
{Field: "year", Operator: "greater_than", Value: "1979"},
|
||||||
|
},
|
||||||
|
})
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("Evaluate: %v", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
// Only "Another One Bites the Dust" (Queen, 1980) satisfies both.
|
||||||
|
if len(tracks) != 1 {
|
||||||
|
t.Fatalf("got %d tracks, want 1", len(tracks))
|
||||||
|
}
|
||||||
|
|
||||||
|
if want := "Another One Bites the Dust"; tracks[0].TrackName != want {
|
||||||
|
t.Errorf("got %q, want %q", tracks[0].TrackName, want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// TestEvaluate_ArtworkEnrichment verifies the presentation-only
|
// TestEvaluate_ArtworkEnrichment verifies the presentation-only
|
||||||
// cover-art and MusicBrainz-ID fields are attached to matched tracks
|
// cover-art and MusicBrainz-ID fields are attached to matched tracks
|
||||||
// by the batched fetchArtwork pass (they are no longer part of the
|
// by the batched fetchArtwork pass (they are no longer part of the
|
||||||
@@ -1340,7 +1519,7 @@ func TestSQLInjection_FieldName(t *testing.T) {
|
|||||||
Field: "title; DROP TABLE playlists",
|
Field: "title; DROP TABLE playlists",
|
||||||
Operator: "is", Value: "x",
|
Operator: "is", Value: "x",
|
||||||
},
|
},
|
||||||
})
|
}, MatchAll)
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal(
|
t.Fatal(
|
||||||
"expected error for injected field name, got nil",
|
"expected error for injected field name, got nil",
|
||||||
|
|||||||
@@ -193,6 +193,10 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
// ── Internal state ──────────────────────────────────────────────
|
// ── Internal state ──────────────────────────────────────────────
|
||||||
|
|
||||||
@state() private ruleRows: RuleRow[] = [emptyRule()];
|
@state() private ruleRows: RuleRow[] = [emptyRule()];
|
||||||
|
/** Whether every rule must hold or any one of them. Mirrors the
|
||||||
|
* backend's `match`; 'all' is the default and the only thing a
|
||||||
|
* playlist saved before this existed can have meant. */
|
||||||
|
@state() private matchType: 'all' | 'any' = 'all';
|
||||||
@state() private limit = 0;
|
@state() private limit = 0;
|
||||||
@state() private sortField = 'random';
|
@state() private sortField = 'random';
|
||||||
@state() private sortDir = '';
|
@state() private sortDir = '';
|
||||||
@@ -224,6 +228,19 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
flex-shrink: 0;
|
flex-shrink: 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
.match-row {
|
||||||
|
display: flex;
|
||||||
|
align-items: center;
|
||||||
|
gap: 6px;
|
||||||
|
font-size: var(--yj-text-sm);
|
||||||
|
color: var(--yj-text-secondary, #b3b3b3);
|
||||||
|
flex-wrap: wrap;
|
||||||
|
}
|
||||||
|
|
||||||
|
.match-select {
|
||||||
|
min-width: 72px;
|
||||||
|
}
|
||||||
|
|
||||||
.rule-row {
|
.rule-row {
|
||||||
display: grid;
|
display: grid;
|
||||||
grid-template-columns: 160px 140px 1fr 28px;
|
grid-template-columns: 160px 140px 1fr 28px;
|
||||||
@@ -511,6 +528,10 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
);
|
);
|
||||||
|
|
||||||
this.ruleRows = rows.length > 0 ? rows : [emptyRule()];
|
this.ruleRows = rows.length > 0 ? rows : [emptyRule()];
|
||||||
|
// A playlist saved before this field existed has no match
|
||||||
|
// and means "all" — the backend reads an empty match the
|
||||||
|
// same way, so an upgrade cannot widen anyone's playlist.
|
||||||
|
this.matchType = parsed.match === 'any' ? 'any' : 'all';
|
||||||
this.limit = parsed.limit ?? 0;
|
this.limit = parsed.limit ?? 0;
|
||||||
this.sortField = parsed.sort_field || 'random';
|
this.sortField = parsed.sort_field || 'random';
|
||||||
this.sortDir = parsed.sort_dir ?? '';
|
this.sortDir = parsed.sort_dir ?? '';
|
||||||
@@ -551,6 +572,7 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
|
|
||||||
return JSON.stringify({
|
return JSON.stringify({
|
||||||
rules,
|
rules,
|
||||||
|
match: this.matchType,
|
||||||
limit: this.limit || 0,
|
limit: this.limit || 0,
|
||||||
sort_field: this.sortField || '',
|
sort_field: this.sortField || '',
|
||||||
sort_dir: this.sortDir || '',
|
sort_dir: this.sortDir || '',
|
||||||
@@ -630,6 +652,11 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
this.onRulesChanged();
|
this.onRulesChanged();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private updateMatchType(value: string) {
|
||||||
|
this.matchType = value === 'any' ? 'any' : 'all';
|
||||||
|
this.onRulesChanged();
|
||||||
|
}
|
||||||
|
|
||||||
private updateSortField(value: string) {
|
private updateSortField(value: string) {
|
||||||
this.sortField = value;
|
this.sortField = value;
|
||||||
if (!value) this.sortDir = '';
|
if (!value) this.sortDir = '';
|
||||||
@@ -731,6 +758,7 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
override render() {
|
override render() {
|
||||||
return html`
|
return html`
|
||||||
<div class="rule-rows">
|
<div class="rule-rows">
|
||||||
|
${this.renderMatchType()}
|
||||||
${this.ruleRows.map((row, index) =>
|
${this.ruleRows.map((row, index) =>
|
||||||
this.renderRuleRow(row, index),
|
this.renderRuleRow(row, index),
|
||||||
)}
|
)}
|
||||||
@@ -743,6 +771,46 @@ export class SmartPlaylistEditor extends LitElement {
|
|||||||
`;
|
`;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Whether every rule has to hold, or any one of them.
|
||||||
|
*
|
||||||
|
* It is a sentence with a control in the middle rather than a
|
||||||
|
* labelled field, because the two readings differ by one word and
|
||||||
|
* that word is the whole of the setting — "Match **all** of the
|
||||||
|
* following rules" says what the list below it means in a way a
|
||||||
|
* select labelled "Match" beside a list does not.
|
||||||
|
*
|
||||||
|
* Hidden while there is one rule: with nothing to combine, all and
|
||||||
|
* any are the same query, and a control whose two settings cannot
|
||||||
|
* differ is a question the user has no way to answer wrongly and
|
||||||
|
* no reason to answer at all.
|
||||||
|
*/
|
||||||
|
private renderMatchType() {
|
||||||
|
if (this.ruleRows.length < 2) return nothing;
|
||||||
|
|
||||||
|
return html`
|
||||||
|
<div class="match-row">
|
||||||
|
<span>Match</span>
|
||||||
|
<select
|
||||||
|
class="match-select"
|
||||||
|
aria-label="Match all or any of the following rules"
|
||||||
|
@change=${(e: Event) =>
|
||||||
|
this.updateMatchType(
|
||||||
|
(e.target as HTMLSelectElement).value,
|
||||||
|
)}
|
||||||
|
>
|
||||||
|
<option value="all" ?selected=${this.matchType === 'all'}>
|
||||||
|
all
|
||||||
|
</option>
|
||||||
|
<option value="any" ?selected=${this.matchType === 'any'}>
|
||||||
|
any
|
||||||
|
</option>
|
||||||
|
</select>
|
||||||
|
<span>of the following rules</span>
|
||||||
|
</div>
|
||||||
|
`;
|
||||||
|
}
|
||||||
|
|
||||||
private renderRuleRow(row: RuleRow, index: number) {
|
private renderRuleRow(row: RuleRow, index: number) {
|
||||||
const isBetween = row.operator === 'between';
|
const isBetween = row.operator === 'between';
|
||||||
const operators = row.field ? getOperatorsForField(row.field) : [];
|
const operators = row.field ? getOperatorsForField(row.field) : [];
|
||||||
|
|||||||
Reference in New Issue
Block a user