fix(Baserow Node): Handle missing filter values - #37324
Conversation
Add test for handling undefined value in formatBaserowFilterValue function.
|
✅ CLA Check passed. All contributors on this PR have signed the n8n CLA — thank you! |
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would auto-approve. Fixes Baserow filter formatting to handle missing values, with a regression test pinning the behavior.
Re-trigger cubic
|
Hey @mukeshr06, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: • Your PR references the GitHub issue it fixes (or, for feature requests, a link to the corresponding community forum post). • Tests are included for any new functionality, logic changes or bug fixes. • The PR aligns with our contribution guidelines. Why the linked issue matters: Our teams pick up work from the issue, not from individual pull requests — the issue is what reaches them, with your PR linked to it. So please make sure the issue contains everything needed to judge the change: a clear problem description, reproduction steps, and the expected behaviour. If the issue is thin, add the missing context there rather than only in the PR description. Regarding new nodes: We no longer accept new nodes directly into the core codebase. Instead, we encourage contributors to follow our Community Node Submission Guide to publish nodes independently. If your node integrates with an AI service that you own or represent, please email nodes@n8n.io and we will be happy to discuss the best approach. About review timelines: While we plan to review it as soon as possible, we are currently unable to provide an exact timeframe. Our goal is to begin reviews within a month, but this may change depending on team priorities. We will reach out when the review begins. Please also note that other contributors may have opened pull requests for the same issue. We keep them all open so the reviewing team can choose the approach that fits best. Once the issue is resolved, the remaining pull requests are closed — this is not a judgement on the quality of your work, and we're grateful for it either way. Thank you again for contributing to n8n. |
|
Before this PR can be triaged, please take a look at the failing checks and fix them. If the failures don't look related to your changes, rebasing onto the latest Once the checks pass, this PR will automatically be picked up for triage again. If you have questions or run into trouble, reply here and we'll help. |
ManyaS-Git
left a comment
There was a problem hiding this comment.
Thanks for the clear write-up and the regression test. The fix is small and correct.
What I like
- Handling it at the helper boundary (
formatBaserowFilterValue) is the right call — it keeps every caller (onlyBaserow.node.ts:393today) free of per-call guards. value?.trim() ?? ''also coversnull, not justundefined, and widening the param tovalue?: stringis backward compatible.- The existing empty-value branch is preserved exactly, so behaviour for an explicit empty string is unchanged.
Suggestions (non-blocking)
- The new test only covers
equalwithundefined. Because theundefinedpath also goes through theDEPRECATED_TIMEZONE_ONLY_OPERATORSbranch (returnstimezone) and the date branches (return''), I'd add two more cases to lock that intent, e.g.formatBaserowFilterValue('date_equals_today', undefined, 'Europe/Berlin')->'Europe/Berlin'andformatBaserowFilterValue('date_is_after', undefined)->''. Cheap insurance against a future refactor silently changing those branches. - Semantics: an
undefinedvalue now formats to''(or the timezone for the legacy date operators). For a date operator that means the query param ends up as an empty string. Worth confirming the Baserow API accepts an empty filter value there — otherwise users trade a crash for a400. If an empty value is undesirable, omitting the filter entirely in the caller could be cleaner, but the helper-level guard is a reasonable, low-risk first step.
Verified
- The caller at
Baserow.node.ts:391-397passesvaluestraight from the filter config, so an unset value really isundefinedand this is the correct place to normalize.
Looks good to me — happy to see it merged once the extra test cases are in (optional).
|
/cla-check |
|
Thanks for picking this up. One gap while this is open: Hit on 2.37.10 — Root cause is the signature asserting a contract the caller doesn't enforce — Still one line, and a superset of the current fix: value?: unknown,
const trimmed = String(value ?? '').trim();
Happy to open a follow-up PR if you'd rather keep this one scoped to the missing-value case. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 4/5
- In
packages/nodes-base/nodes/Baserow/GenericFunctions.ts, coercing object-valued filter expressions to"[object Object]"can produce silently incorrect Baserow filters; format supported values explicitly or fail with a clear validation error.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/nodes-base/nodes/Baserow/GenericFunctions.ts">
<violation number="1" location="packages/nodes-base/nodes/Baserow/GenericFunctions.ts:80">
P3: `String(value ?? '')` silently coerces non-primitive values to `"[object Object]"` instead of formatting them or failing loudly. A filter value bound to an expression that resolves to an object (e.g. `{{ $json.someObject }}`) now produces `filter__field_x__equal=[object Object]` in the Baserow query — no crash, but silently wrong/no results. The previous code at least failed with a clear error. For other objects whose `toString()` throws (or Symbol values), `String()` can still throw. Consider narrowing with explicit type guards (string/number/boolean/DateTime) and handling object-typed values explicitly rather than coercing every `unknown` through `String()`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| timezone = 'UTC', | ||
| ): string { | ||
| const trimmed = value.trim(); | ||
| const trimmed = String(value ?? '').trim(); |
There was a problem hiding this comment.
P3: String(value ?? '') silently coerces non-primitive values to "[object Object]" instead of formatting them or failing loudly. A filter value bound to an expression that resolves to an object (e.g. {{ $json.someObject }}) now produces filter__field_x__equal=[object Object] in the Baserow query — no crash, but silently wrong/no results. The previous code at least failed with a clear error. For other objects whose toString() throws (or Symbol values), String() can still throw. Consider narrowing with explicit type guards (string/number/boolean/DateTime) and handling object-typed values explicitly rather than coercing every unknown through String().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/nodes-base/nodes/Baserow/GenericFunctions.ts, line 80:
<comment>`String(value ?? '')` silently coerces non-primitive values to `"[object Object]"` instead of formatting them or failing loudly. A filter value bound to an expression that resolves to an object (e.g. `{{ $json.someObject }}`) now produces `filter__field_x__equal=[object Object]` in the Baserow query — no crash, but silently wrong/no results. The previous code at least failed with a clear error. For other objects whose `toString()` throws (or Symbol values), `String()` can still throw. Consider narrowing with explicit type guards (string/number/boolean/DateTime) and handling object-typed values explicitly rather than coercing every `unknown` through `String()`.</comment>
<file context>
@@ -74,10 +74,10 @@ function isFullyFormattedMultiStepValue(value: string): boolean {
timezone = 'UTC',
): string {
- const trimmed = value?.trim() ?? '';
+ const trimmed = String(value ?? '').trim();
if (!trimmed) {
</file context>
Summary
Prevent
formatBaserowFilterValue()from throwing when a saved Baserow filter has novalue. The helper now normalizes a missing value to an empty string and follows the existing empty-value branch, preserving the date and timezone formatting behavior.Add focused regression coverage for an
undefinedfilter value. Handling this at the helper boundary keeps callers from needing separate guards.How to test
pnpm --filter n8n-nodes-base test -- nodes/Baserow/__tests__/GenericFunctions.test.ts.master, add the new regression case and observeTypeError: Cannot read properties of undefined (reading 'trim').undefinedand verify execution no longer fails while formatting the filter.Related Linear tickets, Github issues, and Community forum posts
Fixes #36165
Review / Merge checklist
Backport to Beta,Backport to Stable, orBackport to v1(if the PR is an urgent fix that needs to be backported)