feat(SyntheticTimeSeries): only allow Python identifiers as variable names - #2762
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2762 +/- ##
==========================================
- Coverage 93.78% 93.75% -0.03%
==========================================
Files 513 513
Lines 52445 52449 +4
==========================================
- Hits 49184 49176 -8
- Misses 3261 3273 +12
🚀 New features to boost your workflow:
|
…names-in-synthetic-time-series-queries
There was a problem hiding this comment.
Code Review
This pull request introduces a validation check in the synthetic time series expression builder to ensure that variable names are valid Python identifiers, raising a ValueError otherwise. A corresponding unit test has been added to verify this behavior, and the auto-generated synchronous API file has been updated. There are no review comments, and I have no further feedback to provide.
|
This seems to me like a breaking change. I'm happy to let it through regardless though - as synthetic TS see very little use, but lets get some more eyes on this (perhaps a slack thread in #topic-sdk) |
I do not have any urgent need for this, so it can wait to next major release. The non-breaking bug fix part of this is in #2766. |
Description
If you try to make an expression like
"a-b-1", with the variable substitution{"a-b": "ts_ex_id"}, it will currently fail with an unclear error.If you make the expression
"a - a/b", with the variable subsutiotn{"a": "ts_ex_id_1", "a/b": "ts_ex_id_2"}, it will not fail, but incorrectly be translated to the expression"ts{externalId:'ts_ex_id_1'} - ts{externalId:'ts_ex_id_1'}/b".The safest thing to do seems like dissallowing variable names that are not Python identifiers, or we might end up with variable names that also contains mathemtical operators.
This PR explicitly fobids variable names that are not Python identifiers with a clear error message.
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.