Config UI: add param value validation (host) #26149
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Instead of calling
regexp.MatchStringon every validation, consider compiling patterns once when loading templates (and failing fast if a pattern is invalid), then reusing the compiled regex to avoid runtime compilation overhead and late pattern errors. validateParamscurrently treats any non-emptyfmt.Sprintf("%v", v)as a value to validate; if patterns are meant only for string parameters it might be safer to restrict validation to string-typed params or explicitly handle non-string types to avoid surprising behavior if patterns are added to numeric/bool fields in the future.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of calling `regexp.MatchString` on every validation, consider compiling patterns once when loading templates (and failing fast if a pattern is invalid), then reusing the compiled regex to avoid runtime compilation overhead and late pattern errors.
- `validateParams` currently treats any non-empty `fmt.Sprintf("%v", v)` as a value to validate; if patterns are meant only for string parameters it might be safer to restrict validation to string-typed params or explicitly handle non-string types to avoid surprising behavior if patterns are added to numeric/bool fields in the future.
## Individual Comments
### Comment 1
<location> `server/http_config_helper_test.go:183-181` </location>
<code_context>
assert.NotContains(t, result, "outdatedField")
}
+
+func TestValidateHostPattern(t *testing.T) {
+ tests := []struct {
+ host string
+ valid bool
+ }{
+ {"192.168.1.100", true},
+ {"192.168.1.100:8080", true},
+ {"example.com", true},
+ {"http://192.168.1.100", false},
+ {"192.168.1.100/admin", false},
+ {"192.168.1.100 ", false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.host, func(t *testing.T) {
+ conf := map[string]any{
+ "template": "tasmota",
+ "host": tt.host,
+ "usage": "pv",
+ }
+
+ err := validateParams(templates.Meter, conf)
+ if tt.valid {
+ require.NoError(t, err)
+ } else {
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "does not match required pattern")
+ }
+ })
+ }
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add test cases for empty and whitespace-only host values to document that they are allowed to bypass pattern validation.
Since `validateParams` skips validation when the stringified value is empty (`valueStr == ""`), please add tests for `host == ""` and a whitespace-only value like `host == " "`. This will clarify the intended behavior for optional/blank hosts and protect against regressions if `validateParams` changes.
</issue_to_address>
### Comment 2
<location> `server/http_config_helper_test.go:184-187` </location>
<code_context>
}
+
+func TestValidateHostPattern(t *testing.T) {
+ tests := []struct {
+ host string
+ valid bool
+ }{
+ {"192.168.1.100", true},
+ {"192.168.1.100:8080", true},
</code_context>
<issue_to_address>
**suggestion (testing):** Add a dedicated unit test for `validateParams` error handling when the pattern itself is invalid.
Specifically, there should be a test where the template/class includes a `Param` whose `Pattern` is an invalid regex (e.g., an unmatched bracket), and the assertion checks that `validateParams` returns an error containing `"invalid regex pattern"`. This locks in the new behavior and ensures invalid patterns in templates are reported clearly.
Suggested implementation:
```golang
assert.Equal(t, "grid", result["usage"], "usage")
assert.NotContains(t, result, "outdatedField")
}
func TestValidateParams_InvalidPattern(t *testing.T) {
t.Parallel()
// This test verifies that an invalid regex pattern on a Param
// causes validateParams to return an error containing
// "invalid regex pattern".
// NOTE: The exact template/class/param types and validateParams
// signature may differ in your codebase; adjust below accordingly.
// Example shape; replace with your actual types.
type ParamDef struct {
Name string
Pattern string
}
type ClassDef struct {
Name string
Params []ParamDef
}
type TemplateDef struct {
Classes []ClassDef
}
// Construct a template/class with a Param that has an invalid regex pattern.
template := TemplateDef{
Classes: []ClassDef{
{
Name: "test-class",
Params: []ParamDef{
{
Name: "badParam",
Pattern: "[", // invalid regex: unmatched bracket
},
},
},
},
}
params := map[string]any{
"badParam": "some-value",
}
// Call validateParams with the class and params; adjust to real signature.
err := validateParams("test-class", template, params)
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid regex pattern")
}
```
The added test uses placeholder type definitions (`ParamDef`, `ClassDef`, `TemplateDef`) and a guessed `validateParams` signature. To integrate this correctly:
1. Replace the local `ParamDef`, `ClassDef`, and `TemplateDef` types with the actual types used in your production code (or import them if they are in another package).
2. Update the construction of `template` to match how templates/classes/params are represented in your existing code, ensuring one `Param` has `Pattern: "["`.
3. Adjust the `validateParams` call to use the real function signature (e.g., it might be `validateParams(templateName string, className string, params map[string]any)` or accept concrete template/class objects).
4. Keep the assertions the same: `require.Error(t, err)` and `assert.Contains(t, err.Error(), "invalid regex pattern")` so the behavior is locked in as requested.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
andig
reviewed
Dec 22, 2025
Collaborator
|
Is there any way to make it clearer to the user what is wrong with the format they entered? |
2 tasks
Member
Author
@Maschga Browser validation supports showing additional custom information. I've introduced (updated above screenshots) |
andig
reviewed
Dec 22, 2025
andig
reviewed
Dec 22, 2025
andig
reviewed
Dec 22, 2025
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The host regex is currently duplicated in both
defaults.yamlandTestValidatePattern; consider centralizing it (e.g., via a shared constant or loading from the template) to avoid future drift between the implementation and the tests. - In
validatePatternyou callregexp.MatchStringfor every validation, which recompiles the regex each time; if this ends up used on many params/values, consider precompiling and caching the regex (e.g., onPatternor duringTemplate.Validate) to avoid repeated compilation overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The host regex is currently duplicated in both `defaults.yaml` and `TestValidatePattern`; consider centralizing it (e.g., via a shared constant or loading from the template) to avoid future drift between the implementation and the tests.
- In `validatePattern` you call `regexp.MatchString` for every validation, which recompiles the regex each time; if this ends up used on many params/values, consider precompiling and caching the regex (e.g., on `Pattern` or during `Template.Validate`) to avoid repeated compilation overhead.
## Individual Comments
### Comment 1
<location> `util/templates/template.go:397-400` </location>
<code_context>
return nil, nil, fmt.Errorf("missing required `%s`", p.Name)
}
+ // validate pattern if defined
+ if s != "" && p.Pattern.Regex != "" {
+ // convert value to string for validation (remove yaml quotes if present)
+ valueStr := fmt.Sprintf("%v", val)
+ if err := validatePattern(p.Pattern.Regex, valueStr, p.Pattern.Examples); err != nil {
+ return nil, nil, fmt.Errorf("%s: %w", p.Name, err)
</code_context>
<issue_to_address>
**issue (bug_risk):** Pattern validation should likely use the rendered string `s` instead of the raw `val`.
`s` is used to gate validation (`if s != "" && p.Pattern.Regex != ""`), but the regex is run against `valueStr` derived from `val`. When `s` and `val` differ (e.g. trimming, formatting, coercion), the value shown/stored in `res[out]` could pass while the regex check fails, or vice versa. To avoid this mismatch, apply the regex to the same representation that is stored (reuse `s`, or derive `valueStr` from `s`).
</issue_to_address>
### Comment 2
<location> `util/templates/template_test.go:77-87` </location>
<code_context>
require.NoError(t, err)
}
+
+func TestValidatePattern(t *testing.T) {
+ tmpl := &Template{
+ TemplateDefinition: TemplateDefinition{
+ Params: []Param{{Name: "host", Pattern: Pattern{Regex: `^[^\\/\s]+(:[0-9]{1,5})?$`}}},
+ },
+ }
+
+ tests := []struct {
+ host string
+ valid bool
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for an empty host value to assert that pattern validation is skipped for empty (non-required) params.
Since pattern validation only runs when `s != ""`, an empty string should still be treated as valid when the param isn’t required, even if a pattern is set. Please add a test case like `{"", true}` to capture this and guard against regressions if the pattern logic changes later.
```suggestion
tests := []struct {
host string
valid bool
}{
{"", true}, // empty non-required param should be treated as valid even when a pattern is set
{"192.168.1.100", true},
{"192.168.1.100:8080", true},
{"example.com", true},
{"http://192.168.1.100", false},
{"192.168.1.100/admin", false},
{"192.168.1.100 ", false},
}
```
</issue_to_address>
### Comment 3
<location> `util/templates/template_test.go:89-94` </location>
<code_context>
+ {"192.168.1.100 ", false},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.host, func(t *testing.T) {
+ _, _, err := tmpl.RenderResult(RenderModeInstance, map[string]any{"host": tt.host})
+ if tt.valid {
+ require.NoError(t, err)
+ } else {
+ require.Error(t, err)
+ assert.Contains(t, err.Error(), "does not match required pattern")
</code_context>
<issue_to_address>
**suggestion (testing):** Add dedicated tests for `validatePattern` and pattern example validation to exercise error paths and example handling.
Current tests only cover the happy path and a generic `validatePattern` failure via `RenderResult`. To fully cover the new behaviour, please add:
1) A unit test for `validatePattern` with an invalid regex (e.g. `"("`) asserting an `"invalid regex pattern"` error.
2) A `Template.Validate` test where `Pattern.Regex` and `Pattern.Examples` include at least one non-matching example, asserting the `"pattern example ... is invalid"` error.
3) (Optional) A test where `Pattern.Examples` is set and `RenderResult` fails validation, asserting the error includes the examples list to lock in the build-time hint behaviour.
Suggested implementation:
```golang
for _, tt := range tests {
t.Run(tt.host, func(t *testing.T) {
_, _, err := tmpl.RenderResult(RenderModeInstance, map[string]any{"host": tt.host})
if tt.valid {
require.NoError(t, err)
} else {
require.Error(t, err)
assert.Contains(t, err.Error(), "does not match required pattern")
}
})
}
}
func TestValidatePattern_InvalidRegex(t *testing.T) {
t.Parallel()
// NOTE: this assumes Pattern has a Regex field and validatePattern returns an
// "invalid regex pattern" error when the regex cannot be compiled.
p := Pattern{
Regex: "(",
Examples: []string{"example"},
}
err := validatePattern("example", p)
require.Error(t, err)
assert.Contains(t, err.Error(), "invalid regex pattern")
}
func TestTemplateValidate_InvalidPatternExample(t *testing.T) {
t.Parallel()
// NOTE: this assumes Template.Validate validates Pattern.Examples against Pattern.Regex
// and returns an error of the form "pattern example <value> is invalid".
tmpl := &Template{
Parameters: map[string]ParameterSchema{
"host": {
Type: "string",
Pattern: &Pattern{
Regex: `^192\.168\.1\.\d+$`,
Examples: []string{"192.168.1.10", "example.com"},
},
},
},
}
errs := tmpl.Validate()
require.NotEmpty(t, errs)
var found bool
for _, err := range errs {
if strings.Contains(err.Error(), "pattern example") &&
strings.Contains(err.Error(), "example.com") &&
strings.Contains(err.Error(), "is invalid") {
found = true
break
}
}
assert.True(t, found, "expected Template.Validate to report invalid pattern example")
}
func TestRenderResult_PatternExamplesIncludedInError(t *testing.T) {
t.Parallel()
// NOTE: this assumes Template.Validate passes and that RenderResult performs
// runtime validation using the same Pattern and includes the examples in the
// error message as a build-time hint.
tmpl := &Template{
Parameters: map[string]ParameterSchema{
"host": {
Type: "string",
Pattern: &Pattern{
Regex: `^192\.168\.1\.\d+$`,
Examples: []string{"192.168.1.10", "192.168.1.20"},
},
},
},
// add any other required Template fields here so that RenderResult succeeds
}
// Use a value that does not match the regex to trigger validation failure.
_, _, err := tmpl.RenderResult(RenderModeInstance, map[string]any{"host": "example.com"})
require.Error(t, err)
msg := err.Error()
assert.Contains(t, msg, "does not match required pattern")
assert.Contains(t, msg, "192.168.1.10")
assert.Contains(t, msg, "192.168.1.20")
}
```
The above changes assume several details about your existing types and helpers. To integrate them cleanly, you may need to:
1. **Imports**
- Ensure the test file imports `strings`:
```go
import (
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
```
- Skip adding `testing`, `assert`, or `require` if they are already imported.
2. **Pattern / validatePattern shape**
- The `TestValidatePattern_InvalidRegex` test assumes:
```go
type Pattern struct {
Regex string
Examples []string
}
func validatePattern(value string, p Pattern) error
```
- If your `validatePattern` signature or `Pattern` shape differ (e.g., pointer receiver, different field names), adjust the call and struct literal accordingly, and keep the assertion on the `"invalid regex pattern"` substring.
3. **Template / ParameterSchema / Validate / RenderResult shape**
- The `TestTemplateValidate_InvalidPatternExample` and `TestRenderResult_PatternExamplesIncludedInError` tests assume:
```go
type Template struct {
Parameters map[string]ParameterSchema
// other fields required by RenderResult
}
type ParameterSchema struct {
Type string
Pattern *Pattern
// other fields as needed
}
func (t *Template) Validate() []error
func (t *Template) RenderResult(mode RenderMode, inputs map[string]any) (/* ... */, error)
```
- Update field names, types, and the `Validate` return type to match your actual API. If `Validate` returns a single `error` instead of `[]error`, change the assertions accordingly (e.g., `require.Error`, `assert.Contains`).
- If `RenderResult` has a different signature or requires additional `Template` fields (e.g. `Body`, `Name`, etc.), set them in the test `tmpl` construction so it reaches the runtime parameter validation path.
4. **Error message substrings**
- If your implementation uses slightly different wording (for example `"invalid regex"` instead of `"invalid regex pattern"` or `"pattern example \"example.com\" is invalid"` with quotes), update the `assert.Contains` calls to match the actual error text while preserving the intent:
- `validatePattern` must surface that the regex itself is invalid.
- `Template.Validate` must report which example is invalid.
- `RenderResult` must include both the generic pattern failure and the examples list in the error message.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
andig
reviewed
Jan 5, 2026
| } | ||
|
|
||
| // validatePattern checks if a value matches a pattern and returns a descriptive error if not | ||
| func validatePattern(regex, value string, examples []string) error { |
Member
There was a problem hiding this comment.
Die Methode gehört an den Param- spart die Parameter
andig
reviewed
Jan 5, 2026
| } | ||
|
|
||
| // validate the template (only rudimentary for now) | ||
| func (t *Template) Validate() error { |
Member
There was a problem hiding this comment.
Wird Validate nur beim Speichern oder auch beim Config einlesen aufgerufen?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #26135
Add ability to specify a
pattern(regex) to a param definition. If param has a non-empty value the value has to match the pattern. This PR only addshostparam pattern, but we could add others like uri (start withhttp(s)) easily.⚙️ server-side validation in go http api
📱 client-side validation using builtin browse feature (
input[pattern])🧪 introduced
patternexamplesthat act as hint on validation error (see screenshots). also used to validate pattern against examples on build timeNote: Regex handling in Go and browser is generally compatible, but there might be slight differences when it comes to escaping. So it's important to test both when changing or adding new patterns.
Exact validation response UI and messages differs between browsers (see screenshots below), but we get proper focus and highlighting of the affected fields out of the box.
Screenshots
form not submitted yet, show example
browser validation examples
