You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "jscheffl (via GitHub)" <gi...@apache.org> on 2024/08/15 22:06:39 UTC
[PR] Fix UI rendering issues if XCom values are INT, FLOAT or BOOL in web UI [airflow]
jscheffl opened a new pull request, #41516:
URL: https://github.com/apache/airflow/pull/41516
closes: #41514
Before:
After:
--
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] Fix UI rendering issues if XCom values are INT, FLOAT, BOOL or NULL in web UI [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1720782169
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = typeof contentJson === "object"; // ensure numbers/bool are not treated as JSON
Review Comment:
Oh. You are really good. Did not have "NULL" in my test set as in all my examples NULL was never persisted in XCom, in these cases just no XCom was stored. But actually if I try we have even a second problem with NULL in the XCom value. API returns an error so... need to fix this as well.
Added NULL to the party, updated screenshot in PR above how it comes out
--
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] Fix UI rendering issues if XCom values are INT, FLOAT, BOOL or NULL in web UI [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1720822562
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = typeof contentJson === "object"; // ensure numbers/bool are not treated as JSON
Review Comment:
Okay, got a bit more complex now but extracted the utility function and added a test for it.
--
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] Fix UI rendering issues if XCom values are INT, FLOAT or BOOL in web UI [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1719636364
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = Number.isNaN(contentJson); // ensure numbers/bool are not treated as JSON
Review Comment:
I think this actually returns `false` for everything that’s actually JSON…
```js
var c = JSON.stringify({a: 1})
console.log(Number.isNaN(c)) // false
```
--
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] Fix UI rendering issues if XCom values are INT, FLOAT or BOOL in web UI [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1720767743
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = typeof contentJson === "object"; // ensure numbers/bool are not treated as JSON
Review Comment:
This also excludes `null`. I’m not even sure if it’s a possible value here, or how we are expected to treat it.
--
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] Fix UI rendering issues if XCom values are INT, FLOAT or BOOL in web UI [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1720260008
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = Number.isNaN(contentJson); // ensure numbers/bool are not treated as JSON
Review Comment:
Uuups. Yes. Reading docs helps. Still I am wondering why actually it _was_ working with this.
Anyway, how about as I pushed a different approach? After parsing as JSON should be an object (which is true for Array+Dict)
--
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] Fix UI rendering issues if XCom values are INT, FLOAT or BOOL in web UI [airflow]
Posted by "jscheffl (via GitHub)" <gi...@apache.org>.
jscheffl commented on code in PR #41516:
URL: https://github.com/apache/airflow/pull/41516#discussion_r1720782169
##########
airflow/www/static/js/components/RenderedJsonField.tsx:
##########
@@ -47,7 +47,7 @@ const JsonParse = (content: string | object) => {
contentJson = content;
}
contentFormatted = JSON.stringify(contentJson, null, 4);
- isJson = true;
+ isJson = typeof contentJson === "object"; // ensure numbers/bool are not treated as JSON
Review Comment:
Oh. You are really good. Did not have "NULL" in my test set as in all my examples NULL was never persisted in XCom, in these cases just no XCom was stored. But actually if I try we have even a second problem with NULL in the XCom value.
--
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] Fix UI rendering issues if XCom values are INT, FLOAT, BOOL or NULL in web UI [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr merged PR #41516:
URL: https://github.com/apache/airflow/pull/41516
--
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