You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "BeauDevCode (via GitHub)" <gi...@apache.org> on 2026/06/09 05:25:36 UTC
[PR] Make variable JSON validation block invalid saves [airflow]
BeauDevCode opened a new pull request, #68268:
URL: https://github.com/apache/airflow/pull/68268
closes: #68262
## Why
The Variables edit form currently shows invalid JSON-like values as a subtle warning while still allowing the form to be submitted. That makes it easy to save malformed object or array values accidentally.
## What changed
- Turned the existing JSON warning into a form validation error for values that begin with `{` or `[`.
- Reused the existing invalid form state so the Save button is disabled while malformed JSON is present.
- Preserved support for plain string variable values.
- Added component tests for blocking malformed JSON object values and allowing plain string values.
This is intentionally narrower than draft PR #62882: it does not add JSON prettifying or submit-time minification, it only makes the current invalid JSON warning prominent and guarded by validation.
## Tests
- `git diff --check`
- Source sanity checks for the changed TS/TSX files
Not run locally: `pnpm test` / UI lint. This checkout does not have `node_modules`, and this environment does not have `pnpm`, `npm`, or `corepack` available in PATH.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make variable JSON validation block invalid saves [airflow]
Posted by "zachliu (via GitHub)" <gi...@apache.org>.
zachliu commented on PR #68268:
URL: https://github.com/apache/airflow/pull/68268#issuecomment-4672922275
> Thank you for opening the PR.
>
> I think we should make the warning prominent (styling, icon, red borders), but blocking 'save' is too strong I believe.
>
> Other non JSON legitimate values starting with '{' or '[' won't be savable anymore. For instance a Jinja template `{{ var.value.x }}` or plain string such as `[DRAFT] .....`.
fair enough. maybe we can add a toggle or radio button `json`/`text`? when `json`, you can't save a malformed one. when `text`, you can save anything. and the default is `json`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Better warning visibility for invalid JSON value when editing variables [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #68268:
URL: https://github.com/apache/airflow/pull/68268
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make variable JSON validation block invalid saves [airflow]
Posted by "zachliu (via GitHub)" <gi...@apache.org>.
zachliu commented on PR #68268:
URL: https://github.com/apache/airflow/pull/68268#issuecomment-4671726163
+1 on this! we just had a production incident that shows why blocking the save matters.
A malformed Variable (a single trailing comma in JSON) got saved through this form because the `Invalid JSON` warning is easy to miss and `Save` stays enabled. The variable drives parse-time task generation in many of our DAGs, so on the next parse those DAGs serialized with zero tasks, and every scheduled run silently `succeeded` in ~0.1s with all task instances marked removed. Blast radius was far larger than the one variable suggested, and root-causing it took a while because nothing actually errored.
Guarding `Save` behind JSON validation, as in this PR, would have stopped it at the source.
One tangential note in case it's useful: even after we fixed the variable, tasks defined inside a `TaskGroup` stayed `removed`, while bare top-level tasks recovered on reparse. We only got them back by forcing a new serialized DAG version (a structural change) and clearing the runs. That smells like a possible `TaskGroup` reconciliation bug in Airflow 3.x, separate from this UI fix, happy to file a dedicated issue if it'd help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make variable JSON validation block invalid saves [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #68268:
URL: https://github.com/apache/airflow/pull/68268#discussion_r3397251112
##########
airflow-core/src/airflow/ui/src/pages/Variables/ManageVariable/VariableForm.tsx:
##########
@@ -104,11 +104,18 @@ const VariableForm = ({ error, initialVariable, isPending, manageMutate, setErro
<Field.Label fontSize="md">
{translate("columns.value")} <Field.RequiredIndicator />
</Field.Label>
- <Textarea {...field} size="sm" />
+ <Textarea
+ {...field}
+ borderColor={showJsonWarning ? "fg.warning" : undefined}
+ borderWidth={showJsonWarning ? "2px" : undefined}
+ size="sm"
+ />
{showJsonWarning ? (
- <Text color="fg.warning" fontSize="xs">
- {translate("variables.form.invalidJson")}
- </Text>
+ <Box bg="bg.warning" borderColor="fg.warning" borderRadius="md" borderWidth="1px" mt={2} px={3} py={2}>
+ <Text color="fg.warning" fontSize="sm" fontWeight="medium">
+ {translate("variables.form.invalidJson")}
+ </Text>
+ </Box>
Review Comment:
Instead of creating your own component here for an Alert, we can re-use the shared `ui/Alert` component with the appropriate 'Status'.
It's using Chakra Alert underneath:
https://chakra-ui.com/docs/components/alert#status
##########
airflow-core/src/airflow/ui/src/pages/Variables/ManageVariable/VariableForm.tsx:
##########
@@ -104,11 +104,18 @@ const VariableForm = ({ error, initialVariable, isPending, manageMutate, setErro
<Field.Label fontSize="md">
{translate("columns.value")} <Field.RequiredIndicator />
</Field.Label>
- <Textarea {...field} size="sm" />
+ <Textarea
+ {...field}
+ borderColor={showJsonWarning ? "fg.warning" : undefined}
+ borderWidth={showJsonWarning ? "2px" : undefined}
+ size="sm"
+ />
Review Comment:
Maybe don't do that.
It's not consistent with how we hanlde 'warnings' and 'errors' for other inputs/form across the application.
I think that only the Alert bellow will be enough.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make variable JSON validation block invalid saves [airflow]
Posted by "BeauDevCode (via GitHub)" <gi...@apache.org>.
BeauDevCode commented on code in PR #68268:
URL: https://github.com/apache/airflow/pull/68268#discussion_r3399507320
##########
airflow-core/src/airflow/ui/src/pages/Variables/ManageVariable/VariableForm.tsx:
##########
@@ -104,11 +104,18 @@ const VariableForm = ({ error, initialVariable, isPending, manageMutate, setErro
<Field.Label fontSize="md">
{translate("columns.value")} <Field.RequiredIndicator />
</Field.Label>
- <Textarea {...field} size="sm" />
+ <Textarea
+ {...field}
+ borderColor={showJsonWarning ? "fg.warning" : undefined}
+ borderWidth={showJsonWarning ? "2px" : undefined}
+ size="sm"
+ />
{showJsonWarning ? (
- <Text color="fg.warning" fontSize="xs">
- {translate("variables.form.invalidJson")}
- </Text>
+ <Box bg="bg.warning" borderColor="fg.warning" borderRadius="md" borderWidth="1px" mt={2} px={3} py={2}>
+ <Text color="fg.warning" fontSize="sm" fontWeight="medium">
+ {translate("variables.form.invalidJson")}
+ </Text>
+ </Box>
Review Comment:
Thanks for the review.
Updated the warning to use the shared Alert component and removed the warning-specific Textarea styling so it follows existing UI conventions.
I also updated the tests to match the current translation-key behavior and added coverage for Jinja-style values to confirm the warning remains non-blocking for valid non-JSON strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make variable JSON validation block invalid saves [airflow]
Posted by "BeauDevCode (via GitHub)" <gi...@apache.org>.
BeauDevCode commented on PR #68268:
URL: https://github.com/apache/airflow/pull/68268#issuecomment-4673245844
Updated the PR based on the feedback.
The warning is now more prominent, but Save is no longer blocked for values that may be valid non-JSON strings, such as Jinja templates or `[DRAFT]` labels. I also updated the tests to cover the non-blocking behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Better warning visibility for invalid JSON value when editing variables [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #68268:
URL: https://github.com/apache/airflow/pull/68268#issuecomment-4770400256
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org