feat: Extend model name Filter-189

[aviavissar]

What type of PR is this?

What this PR does / why we need it:
we should extend the modelname Filter to be called something like ByField, with configuration of fieldName and optionalValues.

Which issue(s) this PR fixes:

Fixes #

Release note (write NONE if no user-facing change):

NONE

[github-actions]

🚨 Unsigned commits detected! Please sign your commits.

For instructions on how to set up GPG/SSH signing and verify your commits, please see GitHub Documentation.

[noyitz]

the feature itself makes sense and the test coverage looks thorough — nice work on the byField tests and the missing-attribute edge case.

main concern is StringAttribute being defined in the filter package — it's a general-purpose datalayer type and defining it here creates a wrong-direction import dependency for any datasource that needs to populate string attributes. suggest moving it to pkg/framework/interface/datalayer/.

also, the PR body has Fixes # without a number — should reference #189. and the optionalValues nesting around a single field could be simplified to a flat config.

one more thing: the explicit-match path now returns all matching candidates instead of just the first one. this is correct for byField (multiple candidates can share an attribute value) but it's also a behavioral change for the default name-based path. worth noting in the description.

[senanz]

Thanks for extending the model-group filter, @aviavissar — the feature itself (configurable field + attribute-based matching) makes sense and the test coverage is solid.

Two issues to address before merge:

1. [High] StringAttribute lives in the wrong package — it's a general-purpose datalayer type but defined in the filter package, creating wrong-direction import dependencies for any datasource that needs to write the attribute. Move it to pkg/framework/interface/datalayer/.

2. [Medium] optionalValues nesting is premature — wraps a single field, adds complexity to config and code without benefit today. Flatten to a top-level byField.

Also noting that noy raised both of these points and I fully agree with that analysis. The explicit-match path now returning all matches (not just the first) is also a behavioral change from PR #227 worth calling out in the description.

[senanz]

The OptionalValues struct wraps a single field (ByField). This adds an extra nesting level in the config, an extra struct, and a nil-check in the factory — all for zero current benefit.

A flat config is simpler for operators and for the code:

{"fieldName": "anthropic_version", "byField": "anthropicVersion"}

type ModelGroupFilterConfig struct {
    FieldName string `json:"fieldName,omitempty"`
    ByField   string `json:"byField,omitempty"`
}

If future optional parameters are needed, you can add them to the flat struct — or introduce a nested struct at that time when there's an actual grouping rationale. Premature nesting increases cognitive load for config authors today without solving a real problem.

Also note: the README says "The parameters are a flat JSON object with two optional keys" and then shows a nested example — that's contradictory. Flattening the code would make the docs match reality.

[senanz]

@ronenkat - I know you also working on this area, could you please take look so we will not have overlapping ?

[ronenkat]

@ronenkat - I know you also working on this area, could you please take look so we will not have overlapping ?

Discussed with @aviavissar directly.
We agreed that a new filter would be introduced to enable ByAttribute filtering independent of the current model name filtering.

[szedan-rh]

Thanks for the refactor, @aviavissar. The decision to extract a standalone byfieldattribute package (per the discussion with @ronenkat) is the right call — it cleanly separates attribute-based matching from the group/auto semantics in modelgroup. The StringAttribute placement in datalayer/ addresses the layering concern @noyitz raised. The flat config is clean.

That said, I have several issues to flag before this can merge:

---

[High] Behavioral change: multi-match vs single-match for model-name-filter

The original modelname.ModelNameFilter (removed by #227) returned the first matching candidate and stopped:

for _, model := range models {
    if model.GetName() == requested {
        return []datalayer.Model{model}  // early return
    }
}

The modelgroup filter's explicit-name path does the same thing.

The new byfieldattribute-based model-name-filter accumulates all matches:

for _, model := range models {
    value, found := f.candidateValue(model)
    if !found { continue }
    if value == requested {
        result = append(result, model)
    }
}

For the name-based path (no byField), model names should be unique — so in practice this is unlikely to diverge. But it's still a semantic contract change. If any downstream pipeline or picker assumes exactly one result from model-name-filter, this could cause subtle issues.

Ask: Either (a) restore early-return semantics for the model-name-filter type specifically (the by-field-filter multi-match is correct), or (b) explicitly document this behavioral change in the PR description and README. I'd prefer (a) since it's zero-risk backward compatibility.

---

[High] Pipeline composition guidance: when to use which filter

With this PR merged, operators have three filter types to choose from:

Type	Handles
model-group-name-filter	exact name (single match) + auto + auto/<group>
model-name-filter	exact name match (configurable field, all matches)
by-field-filter	attribute-based match (configurable field + attribute)

The README for byfieldattribute doesn't address the relationship with model-group-name-filter. An operator reading the docs might configure both model-group-name-filter and model-name-filter in the same pipeline (both read the model field), causing double filtering or conflicting results.

Ask: Add a "When to use this vs model-group-name-filter" section to the README. At minimum, state that model-name-filter and model-group-name-filter are mutually exclusive for the same field, and by-field-filter is for non-model fields or attribute-based routing.

---

[Medium] Gratuitous godoc reduction in attributemap.go

The diff strips detailed godoc from the AttributeMap interface methods and ReadAttributeKey. These are public API surfaces consumed by plugin authors — the existing comments are genuinely helpful (e.g., explaining Clone behavior on Get, type parameter semantics on ReadAttributeKey). Trimming them provides no functional benefit and reduces discoverability for downstream developers.

Ask: Keep the original godoc for the interface methods. The StringAttribute addition is fine and can land alongside the existing comments.

---

[Medium] Exported type alias leaks implementation detail

type StringAttribute = datalayer.StringAttribute

This re-exports datalayer.StringAttribute from the filter package. Any consumer that writes byfieldattribute.StringAttribute(...) is now coupled to this package for a datalayer primitive. The alias is only used in the compile-time check and tests.

Ask: Either make it unexported (type stringAttribute = datalayer.StringAttribute — but this doesn't work for type aliases), or simply remove the alias and use datalayer.StringAttribute directly in tests. The var _ datalayer.Cloneable = datalayer.StringAttribute("") compile-time check belongs in the datalayer package's own test file, not here.

---

[Low] defaultByFieldValue constant

const defaultByFieldValue = ""

Used only in comparisons like f.byField == defaultByFieldValue. An empty-string sentinel is idiomatic Go and doesn't benefit from a named constant — it's clear what "" means in context. Not blocking, but it adds a layer of indirection without improving readability.

---

[Low] PR hygiene

1. PR title ("feat: Extend model name Filter-189") doesn't match the commit message ("feat: add byfieldattribute filter"). Pick one.
2. Fixes # in the body is empty — should reference #189.
3. Release note says NONE — this introduces a new user-facing plugin type (by-field-filter). That warrants a release note.
4. The README states the pipeline "rejects the request with HTTP 429" when no candidates match. The filter itself returns an empty slice; the 429 is pipeline behavior. This wording implies the filter controls the status code, which is misleading. Reword to "the pipeline rejects the request (typically 429)".

---

Summary

The core design is sound. The main technical concern is the multi-match behavioral change for the backward-compat model-name-filter shim and the missing guidance on filter composition. Address those and this is good to merge.

[noyitz]

looks good after the rework. StringAttribute moved to the datalayer package, config flattened, clean split between model-name-filter (backward compat) and by-field-filter (generic). thanks for addressing the feedback.

[noyitz]

/lgtm

[ronenkat]

Nice. Thank you.
Please review the backward compatibility comment.

[szedan-rh]

@ronenkat can you please run the workflows ?

[github-actions]

This PR is marked as stale after 21d of inactivity. After an additional 14d of inactivity (7d to become rotten, then 7d more), it will be closed. To prevent this PR from being closed, add a comment or remove the lifecycle/stale label.