feat: validate promoted properties and enhance Scriban array access - #154
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds document update validation for promoted properties paths and improves Scriban template array member resolution. The first change validates promoted property keys, values, and path syntax (JSON JSONPath or XML XPath patterns) before applying updates. The second introduces a SmartArray wrapper enabling templates to access array element fields directly. ChangesDocument Promoted Properties Validation
SmartArray Scriban Template Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
SW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs (1)
111-131: ⚖️ Poor tradeoffSmartArray implementation is correct but consider adding diagnostics.
The delegation logic correctly handles edge cases (empty arrays, non-object elements, existing array members). However, the implicit member delegation can make debugging difficult for template authors.
Consider adding diagnostic logging or a template context variable that tracks when delegation occurs, helping developers understand when they're accessing
array.fieldvsarray[0].field.💡 Example: Add optional delegation tracking
public override bool TryGetValue(TemplateContext context, SourceSpan span, string member, out object? value) { if (base.TryGetValue(context, span, member, out value)) return true; if (Count > 0 && this[0] is ScriptObject first) + { + // Optional: Track or log delegation for debugging + // context.SetValue("__delegated_member_access", true); return first.TryGetValue(context, span, member, out value); + } value = null; return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs` around lines 111 - 131, Add optional diagnostics to SmartArray to make implicit delegation visible: modify the SmartArray class (the override of TryGetValue) to record when delegation occurs (i.e., when Count > 0 and this[0] is ScriptObject and you call first.TryGetValue) by emitting a diagnostic via the existing logging/telemetry mechanism or by setting a flag on the TemplateContext/ScriptObject (or pushing a special context variable) so templates can inspect it; ensure the diagnostic is optional/configurable (off by default) and does not change TryGetValue behavior or return values, and reference SmartArray, TryGetValue, ScriptArray, and ScriptObject when adding the hook so reviewers can find the change quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SW.Bitween.Api/Resources/Documents/Update.cs`:
- Around line 61-65: The current validation rejects bracket-based array access
like "items[0].name"; update the regex used in the non-'$' branch in Update.cs
(where pp.Value.Trim() is validated and the SWValidationException is thrown) to
allow square-bracket numeric indexes. Replace the existing pattern
@"^[a-zA-Z_][a-zA-Z0-9_.]*$" with a pattern that permits dot-separated
identifiers and numeric bracket access, e.g.
@"^[a-zA-Z_][a-zA-Z0-9_]*(?:(\.[a-zA-Z_][a-zA-Z0-9_]*)|(\[[0-9]+\]))*$", so
paths like items[0].name are accepted while keeping the same overall validation
and error path (the throw of SWValidationException for invalid promoted property
paths).
In `@SW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs`:
- Line 106: The conversion of JArray to SmartArray in the ToScribanValue mapping
(JArray a => new SmartArray(a.Select(ToScribanValue))) can silently drop data
when templates use member access and only the first element is returned; update
the JArray handling in ToScribanValue/ToScribanValueEnumerable to detect
multi-element arrays and either (a) wrap them in a collection type that
preserves all elements for iteration, (b) log/warn via the existing logging
facility when a SmartArray is created from an array with Count > 1, or (c) throw
a clear exception to force template authors to handle multi-element arrays
explicitly; reference JArray, SmartArray, and ToScribanValue when making the
change so reviewers can find the mapping and then add the chosen
validation/logging behavior and update any docs/comments accordingly.
---
Nitpick comments:
In `@SW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs`:
- Around line 111-131: Add optional diagnostics to SmartArray to make implicit
delegation visible: modify the SmartArray class (the override of TryGetValue) to
record when delegation occurs (i.e., when Count > 0 and this[0] is ScriptObject
and you call first.TryGetValue) by emitting a diagnostic via the existing
logging/telemetry mechanism or by setting a flag on the
TemplateContext/ScriptObject (or pushing a special context variable) so
templates can inspect it; ensure the diagnostic is optional/configurable (off by
default) and does not change TryGetValue behavior or return values, and
reference SmartArray, TryGetValue, ScriptArray, and ScriptObject when adding the
hook so reviewers can find the change quickly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86665962-abd6-408c-8395-96fe0199a37f
📒 Files selected for processing (2)
SW.Bitween.Api/Resources/Documents/Update.csSW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs
| { | ||
| JObject o => BuildScriptObject(o), | ||
| JArray a => a.Select(ToScribanValue).ToList(), | ||
| JArray a => new SmartArray(a.Select(ToScribanValue)), |
There was a problem hiding this comment.
Risk of silent data loss when accessing multi-element arrays.
Converting all JArray instances to SmartArray enables data.field syntax for array elements, but when an array contains multiple elements, only the first element is accessed. Template authors might not realize they're working with a multi-element array, leading to incomplete data processing.
Consider adding validation or logging when SmartArray member access is used on arrays with more than one element, or document this limitation prominently in template guidelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SW.Bitween.NativeAdapters/JsonMapper/ScribanJsonHelper.cs` at line 106, The
conversion of JArray to SmartArray in the ToScribanValue mapping (JArray a =>
new SmartArray(a.Select(ToScribanValue))) can silently drop data when templates
use member access and only the first element is returned; update the JArray
handling in ToScribanValue/ToScribanValueEnumerable to detect multi-element
arrays and either (a) wrap them in a collection type that preserves all elements
for iteration, (b) log/warn via the existing logging facility when a SmartArray
is created from an array with Count > 1, or (c) throw a clear exception to force
template authors to handle multi-element arrays explicitly; reference JArray,
SmartArray, and ToScribanValue when making the change so reviewers can find the
mapping and then add the chosen validation/logging behavior and update any
docs/comments accordingly.
2 participants
Summary by CodeRabbit
Bug Fixes
Improvements