reject file path input defaults#3094
Conversation
markphelps
left a comment
markphelps
left a comment
There was a problem hiding this comment.
Thanks for the fix — the main Path/File default handling looks pointed in the right direction, and the direct Input(...) cases are covered well. I found one regression in the static-method/reference path that changes behavior for non-file inputs, so I think this needs a small tweak before merge. The other comments are test/doc follow-ups that should make this safer to maintain.
| resolved.DefaultUnresolved = false | ||
| } else { | ||
| none := schema.DefaultValue{Kind: schema.DefaultNone} | ||
| resolved.Default = &none |
There was a problem hiding this comment.
Nice catch overall. I think this branch needs one small fix here before it lands: this else changes behavior outside the Path/File case.
resolved starts as a copy of methodInfo.BaseInfo, so it can already have a default from the helper method. Before this PR, if a call-site default was not a literal, we left that base default alone. Now we replace it with None.
I verified this with a non-file input like:
MY = "base.png"
class Helper:
@staticmethod
def img(default: str) -> Input:
return Input(description="Image", default="base.png")
class Predictor(BasePredictor):
def predict(self, image: str = Helper.img(default=MY)) -> str:
return "ok"On this branch the parsed default becomes None; before the PR it stayed "base.png". That is a silent schema change for non-file inputs.
Could we keep the new unresolved marker without clobbering the base default?
} else {
// Preserve the base default for non-file types; file/path types are still
// rejected by validateInputFieldWithInfo via DefaultUnresolved.
resolved.DefaultUnresolved = true
}I checked this locally: it preserves the old non-file behavior, and file/path defaults through this path still get rejected because validateInputFieldWithInfo sees DefaultUnresolved.
| require.False(t, scale.IsRequired()) | ||
| } | ||
|
|
||
| func TestPathAndFileInputsRejectDefaults(t *testing.T) { |
There was a problem hiding this comment.
Can we add one regression test for the static-method/reference path too? The new table is good for direct Input(...), but it would not have caught the default-clobbering bug above because that lives in resolveInputReference.
A useful test would be the non-file helper-method case from the other comment: helper returns Input(default="base.png"), caller passes default=MY, and we assert the parsed default is still "base.png".
It would also be worth adding one Path/File case through this path, e.g. image: Path = Helper.img(default=Path("x.png")), to prove the new DefaultUnresolved rejection still works for registry references.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| se := parseErr(t, tt.source, "Predictor", schema.ModePredict) | ||
| require.Equal(t, tt.wantErr, se.Kind) |
There was a problem hiding this comment.
Small optional tightening: these assertions would be a little stronger if the table also checked a message substring.
The Kind assertion is meaningful, but a wantMsg column would pin the two user-facing paths too:
- literal Path/File default:
defaults are not supported on Path or File inputs - unresolvable constructor default:
Path and File defaults cannot be statically resolved
Not a blocker, just a cheap way to make the regression tests more explicit.
| } | ||
|
|
||
| // InputTypeContainsFileOrPath reports whether a recursive input type contains | ||
| // a Path or File primitive. It is used by parser frontends that need to reject |
There was a problem hiding this comment.
Tiny doc nit: this helper is also used by ValidateInputField through inputFieldContainsFileOrPath, not just by parser frontends.
Maybe:
// It is used by ValidateInputField and parser frontends that need to reject
// unsupported file/path defaults after annotation resolution.Optional, but it would make the comment match the actual call sites.
2 participants
summary
Nonedefaults forPathand deprecatedFileinputs during static schema parsingdefault=Noneworking for optional file/path inputsInput(default=...)expressions soPath("...")defaults no longer silently becomeNonewhy
defaultfor files/paths #606validation
go test ./pkg/schema/python -run 'TestPathAndFileInputsRejectDefaults|TestOptionalInputPipeNone|TestMultipleInputsWithDefaults' -count=1go test ./pkg/schema/... -count=1go test ./pkg/config ./pkg/util -count=1git diff --checknote
go test ./pkg/... -count=1was also attempted. The schema/config/util packages passed, but Docker-dependent packages failed because this machine cannot connect to a Docker daemon atunix:///var/run/docker.sock/ rootless Docker is unavailable.