feat: support search, filtering and grouping for list default configs#879
feat: support search, filtering and grouping for list default configs#879ayushjain17 wants to merge 2 commits intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughIntroduces a new ListGroupedDefaultConfigs operation for listing default configurations with optional grouping, filtering (by name list, prefix, search), and sorting (by key, created date, last modified date). Extends all SDK clients, adds frontend UI with grouping toggle, search, sorting columns, and implements backend logic for both grouped and ungrouped list responses. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/frontend/src/query_updater.rs (1)
152-182:⚠️ Potential issue | 🔴 CriticalBug:
location.searchincludes the leading?, which breaks param matching and produces malformed URLs.Two issues in this implementation:
?prefix not stripped:location.searchreturns the query string with a leading?(as evidenced byuse_query_string()at Line 147 stripping it). Splitting"?page=1&sort=asc"on"&"yields["?page=1", "sort=asc"]. Nowstarts_with("page=")never matches"?page=1", so the param is appended as a duplicate instead of updated. The final result also gets a double??.Empty query string: Splitting
""on"&"yields[""]. After pushing a new param, joining gives"¶m=value", producing"?¶m=value".Proposed fix
pub fn use_update_url_query() -> impl Fn(&[(&str, Option<String>)]) -> String { let location = use_location(); move |input: &[(&str, Option<String>)]| { - let mut params = location + let search = location .search - .get_untracked() - .split("&") + .get_untracked(); + let search = search.strip_prefix('?').unwrap_or(&search); + let mut params: Vec<String> = search + .split('&') .map(String::from) + .filter(|s| !s.is_empty()) .collect::<Vec<_>>();docs/docs/api/Superposition.openapi.json (1)
1231-1263:⚠️ Potential issue | 🟡 MinorClarify the semantic difference between
searchandprefixparameters, or align filter parameters between list endpoints.The ungrouped endpoint
/default-configexposessearch(string) while the grouped endpoint/default-config?grouped=trueusesprefix(string) instead. Both Smithy and OpenAPI spec confirm this divergence exists. Ifsearchandprefixserve different purposes by design, this should be documented to explain why the parameters differ between modes. If they serve the same purpose, align the parameter names to prevent confusion when SDKs or UI code switches between grouped and ungrouped modes.
🤖 Fix all issues with AI agents
In
`@clients/java/sdk/src/main/java/io/juspay/superposition/model/ListGroupedDefaultConfigsInput.java`:
- Around line 219-231: The toBuilder() method in ListGroupedDefaultConfigsInput
calls builder.count(this.count), builder.page(this.page) and
builder.all(this.all) but those fields are nullable Integer/Boolean and the
Builder setters expect primitives, causing NPE on unboxing; update the
toBuilder() in ListGroupedDefaultConfigsInput to guard nullable fields (count,
page, all) with null checks and only call the primitive setters when the
corresponding field is non-null (e.g., if (this.count != null)
builder.count(this.count);), or alternatively modify the Builder API to accept
boxed Integer/Boolean and use those setters; apply the same null-safe pattern
for similar generated Input classes (e.g., ListDefaultConfigsInput) where
toBuilder() currently unboxes nullable fields.
In `@clients/javascript/sdk/src/protocols/Aws_restJson1.ts`:
- Around line 1626-1629: The query map in Aws_restJson1.ts currently uses sparse
array literals like "[, input[_sb]!]" for entries keyed by symbols such as _n,
_sb, _so, and _s; replace these sparse tuples with explicit condition/value
functions to avoid sparse arrays (e.g., emit [() => input[_sb] !== void 0, () =>
input[_sb]!]) so each entry becomes a pair of functions (condition and value)
rather than a sparse array; update the entries for _n, _sb, _so, and _s (and the
similar block at lines ~1775-1782) to use the () => checks and () => value form.
In `@clients/python/sdk/superposition_sdk/_private/schemas.py`:
- Around line 7493-7504: The LIST_GROUPED_DEFAULT_CONFIGS operation currently
embeds the grouped query param in the smithy.api#http trait's "uri"
("/default-config?grouped=true"); change the http trait value to use uri
"/default-config" and instead add an `@httpQuery`("grouped") member on the
operation's input structure (so that the grouped flag is modeled as a query
member and serialized/percent-encoded correctly), then regenerate the SDK so the
Schema (LIST_GROUPED_DEFAULT_CONFIGS) and generated client code reflect the new
`@httpQuery` binding rather than a query string in the http URI.
In `@clients/python/sdk/superposition_sdk/models.py`:
- Around line 7774-7778: The list literal assigned to effective_auth_schemes is
missing a comma between its ShapeID elements causing a SyntaxError; update the
list in models.py (the effective_auth_schemes assignment) to separate entries
with a comma and correct indentation, e.g. ensure
ShapeID("smithy.api#httpBasicAuth"), ShapeID("smithy.api#httpBearerAuth") are
both elements in the list and the closing bracket/parenthesis matches the
surrounding code structure.
In `@crates/context_aware_config/src/api/default_config/handlers.rs`:
- Around line 459-466: The sort currently treats None dates as Less which places
"unknown date" groups first; change the comparator in group_entries.sort_by (the
closure using a_date, b_date and apply_sort_order) so that None is considered
greater than Some (i.e., sorts last) regardless of ascending/descending; update
the match arms involving (None, Some(_)) and (Some(_), None) to return
Ordering::Greater and Ordering::Less respectively (and keep the tie-breaker on
a_key when both None) so apply_sort_order(sort_by, ord) yields the UX of unknown
dates appearing after known dates.
- Around line 581-597: The in-memory pagination block (building
PaginatedResponse when show_all is false) uses offset and count unsafely —
casting a negative offset to usize and allowing count==0 leads to wraparound and
infinite total_pages; clamp or sanitize the incoming pagination params (page and
count) to be at least 1 before computing offset and count, then recompute offset
= (page - 1) * count and use checked/constrained arithmetic when deriving
start/end and total_pages; apply the same sanitization for the DB pagination
path (the other handler that builds PaginatedResponse) so both in-memory and DB
code paths use non-zero, non-negative page/count values and avoid usize casts of
negative offsets.
- Around line 643-648: The search string used in the ilike pattern isn't
escaping SQL LIKE metacharacters, so '%' or '_' in filters.search will act as
wildcards; before building the pattern in handlers.rs, escape backslashes, '%'
and '_' in filters.search (e.g. replace '\' -> '\\', '%' -> '\%', '_' -> '\_'),
then build the pattern as format!("%{}%", escaped_search) and use that pattern
in builder.filter(dsl::key.ilike(pattern)); ensure the escape sequences are
applied to filters.search (the same variable referenced in the current if-let
branch) so unwanted wildcard matches are prevented.
In `@crates/frontend/src/pages/default_config_list.rs`:
- Around line 101-112: The closure redirect_url currently calls
use_update_url_query() inside the closure body which violates Leptos' hook call
timing; hoist the call to use_update_url_query() to component initialization
(top-level of the component function) and store its returned function/handle in
a variable (e.g., update_query_fn), then have redirect_url capture and call that
variable along with pagination_params_rws.get() and the provided prefix; this
ensures use_update_url_query() is invoked during component setup rather than
lazily inside redirect_url.
In `@crates/superposition_sdk/src/operation/list_default_configs.rs`:
- Around line 154-174: The tests and callers still send name as a single string
but the ListDefaultConfigs request now expects name to be an array and
serializes each element with query.push_kv("name", ...); update all usages
(tests, frontend callers, any direct API calls) that build the request for
ListDefaultConfigs (the _input.name field in
crates/superposition_sdk/src/operation/list_default_configs.rs) to pass name as
an array of strings (e.g., ["testConfigKey"]) instead of a single string so the
generated loop that calls query.push_kv("name", ...) emits one query param per
element.
In `@docs/docs/api/list-grouped-default-configs.api.mdx`:
- Around line 1-12: The front-matter field "api" in
docs/docs/api/list-grouped-default-configs.api.mdx contains a large blob that
Gitleaks flagged as a potential secret; verify whether this is a generated,
non-sensitive artifact and if so move the blob out of source-controlled
front-matter (e.g., into a generated artifacts directory or CI-generated file)
or add it to your approved allowlist for generated payloads, otherwise remove or
rotate the secret and update the "api" front-matter in the
ListGroupedDefaultConfigs doc accordingly.
In `@docs/docs/api/Superposition.openapi.json`:
- Around line 1544-1652: The OpenAPI path key "/default-config?grouped=true" is
invalid because paths must not contain query strings; update the spec by moving
the grouped flag into a query parameter on the existing "/default-config" path
(add a "grouped" query parameter with type boolean to the operation with
operationId "ListGroupedDefaultConfigs" and keep its responses/schema
referencing "ListGroupedDefaultConfigsResponseContent"), or alternatively expose
a separate path like "/default-config/grouped" and keep the operationId and
responses as-is; ensure you remove the "?grouped=true" from the path key so the
spec validates.
🧹 Nitpick comments (12)
crates/frontend/src/pages/default_config_list/filter.rs (3)
24-32:filter_index:Vec::removepanics on out-of-bounds index.If
index >= items.0.len(),items.0.remove(index)will panic at runtime. While this is unlikely in practice (the index comes from pill rendering), a bounds check would be defensive.🛡️ Optional defensive fix
fn filter_index<T: Display + FromStr + Clone>( items: &Option<CommaSeparatedQParams<T>>, index: usize, ) -> Option<CommaSeparatedQParams<T>> { items.clone().and_then(|mut items| { - items.0.remove(index); + if index < items.0.len() { + items.0.remove(index); + } (!items.is_empty()).then_some(items) }) }
123-137: No-opon_deletecallback allocated on every render cycle.Since
deletable=false, theCallback::new(|_: String| {})is never invoked. Consider whetherGrayPillcan accept anOption<Callback>foron_deleteto avoid the allocation, or hoist the callback to aletbinding outside the closure to create it once.
150-163: Parsing logic is correct; minor simplification possible.The nested
.then(|| ...).flatten()can be simplified. Consider:♻️ Optional simplification
on:change=move |event| { let names = event_target_value(&event); - let names = (!names.is_empty()) - .then(|| { - let name_list: Vec<String> = names - .split(',') - .map(|s| s.trim().to_string()) - .filter(|s| !s.is_empty()) - .collect(); - (!name_list.is_empty()) - .then_some(CommaSeparatedQParams(name_list)) - }) - .flatten(); + let name_list: Vec<String> = names + .split(',') + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + .collect(); + let names = (!name_list.is_empty()) + .then_some(CommaSeparatedQParams(name_list)); filters_buffer_rws.update(|filter| filter.set_name(names)); }The outer
!names.is_empty()guard is redundant — splitting an empty string by','yields[""], which is filtered out by.filter(|s| !s.is_empty()), resulting in an emptyVecandNonefromthen_some.crates/superposition_types/src/api/default_config.rs (1)
50-69:set_searchdoesn't managegroupedstate, unlikeset_name.
set_nameinternally managesgrouped(clears it whenprefixisNone), butset_searchleavesgroupeduntouched, requiring callers to separately set it. This asymmetry can lead to subtle bugs ifset_searchis called from a new call-site without also updatinggrouped. Consider havingset_searchalso manage thegroupedfield internally for consistency.♻️ Suggested change
pub fn set_search(&mut self, search: Option<String>) { + self.grouped = search.is_none().then_some(true); self.search = search; self.name = None; }crates/frontend/src/pages/default_config_list.rs (3)
32-49: Sort direction flips even when switching to a different column.When clicking a new column header (e.g., switching from
KeytoCreatedAt),flip()inverts the previous column's direction instead of starting with a default (e.g., ascending). A common UX pattern is to reset to ascending when the sorted column changes, and only flip when re-clicking the same column.♻️ Suggested change
fn sort_callback( sort_on: SortOn, filters_rws: RwSignal<DefaultConfigFilters>, pagination_params_rws: RwSignal<PaginationParams>, ) -> Callback<()> { Callback::new(move |_| { let filters = filters_rws.get(); - let sort_by = filters.sort_by.unwrap_or_default().flip(); + let sort_by = if filters.sort_on == Some(sort_on) { + filters.sort_by.unwrap_or_default().flip() + } else { + SortBy::default() + }; let new_filters = DefaultConfigFilters { sort_on: Some(sort_on), sort_by: Some(sort_by), ..filters }; pagination_params_rws.update(|f| f.reset_page()); filters_rws.set(new_filters); }) }
248-250: Debug logging left in the search handler.This
logging::log!call fires on every debounced keystroke. Consider removing it or gating it behind a debug-only flag to keep the browser console clean in production.
260-282: Toggling grouping resets all filters, which may surprise users.Line 266 resets the entire
DefaultConfigFilterstodefault()before togglinggrouped. This clearsprefix,search,sort_by,sort_on, andname. If a user has navigated into a prefix and then toggles grouping, they lose their prefix context. If this is intentional, a brief comment explaining the rationale would help future readers.crates/frontend/src/api.rs (1)
464-506: BorrowDefaultConfigFiltersto match thelist()function signature.The
to_query_param()trait method takes&self(line 19 ofcrates/superposition_types/src/custom_query.rs), and the function body only reads from the filters parameter—it never requires ownership. Changing the signature to borrow the parameter maintains consistency with the similarlist()function (line 438) which borrowsfilters.♻️ Suggested signature change
- pub async fn list_grouped( - pagination: &PaginationParams, - filters: DefaultConfigFilters, + pub async fn list_grouped( + pagination: &PaginationParams, + filters: &DefaultConfigFilters, workspace: &str, org_id: &str, ) -> Result<PaginatedResponse<ListDefaultConfigResponse>, String> {crates/context_aware_config/src/api/default_config/helpers.rs (1)
60-74: Avoid consuming the map inget_from_unflattened_map.Taking ownership forces clones at call sites; a borrowed signature keeps reuse cheap and avoids unnecessary allocations.
♻️ Proposed refactor
-pub(super) fn get_from_unflattened_map( - unflattened_map: InternalStructure, - flattened_key: &str, -) -> Option<InternalStructure> { - let mut current_value = Some(unflattened_map); +pub(super) fn get_from_unflattened_map( + unflattened_map: &InternalStructure, + flattened_key: &str, +) -> Option<&InternalStructure> { + let mut current_value = Some(unflattened_map); @@ - for part in flattened_key.split('.') { - current_value = current_value.and_then(|v| v.sub_keys.get(part).cloned()) - } + for part in flattened_key.split('.') { + current_value = current_value.and_then(|v| v.sub_keys.get(part)) + } current_value }Update tests to pass
&unflattenedand compare against references.clients/python/sdk/superposition_sdk/serialize.py (1)
1827-1835: Guard againstnamebeing a string.If callers pass a single string, this will expand into per-character query params. Consider normalizing to a list to preserve backward compatibility (and apply the same in
list_grouped_default_configs).♻️ Proposed guard
- if input.name is not None: - query_params.extend(("name", e) for e in input.name) + if input.name is not None: + names = [input.name] if isinstance(input.name, str) else input.name + query_params.extend(("name", e) for e in names)crates/context_aware_config/src/api/default_config/handlers.rs (1)
537-539:list_grouped_configsfetches the entiredefault_configstable into memory.This loads all rows without any DB-level filtering, then builds the tree and filters/paginates in Rust. While this is inherent to the unflattening design, it becomes a scalability concern for workspaces with a large number of default config keys (high memory use, slow response times).
Consider at least applying the
namefilter at the DB level (e.g.,WHERE key LIKE ANY('{name1.%, name2.%}')) to reduce the initial data set when name filters are provided, even if the tree still needs to be built from the filtered set.clients/python/sdk/superposition_sdk/models.py (1)
7676-7676: RedundantUnionwrapper with|syntax.
Union[A | B | C]mixes two union syntaxes. Either useUnion[A, B, C]or justA | B | C. Both work, but combining them is non-idiomatic.Suggested fix
-GroupedDefaultConfig = Union[GroupedDefaultConfigGroup | GroupedDefaultConfigConfig | GroupedDefaultConfigUnknown] +GroupedDefaultConfig = GroupedDefaultConfigGroup | GroupedDefaultConfigConfig | GroupedDefaultConfigUnknown
...nts/java/sdk/src/main/java/io/juspay/superposition/model/ListGroupedDefaultConfigsInput.java
Show resolved
Hide resolved
| LIST_GROUPED_DEFAULT_CONFIGS = Schema( | ||
| id=ShapeID("io.superposition#ListGroupedDefaultConfigs"), | ||
| shape_type=ShapeType.OPERATION, | ||
| traits=[ | ||
| Trait.new(id=ShapeID("smithy.api#tags"), value=( | ||
| "Default Configuration", | ||
| )), | ||
| Trait.new(id=ShapeID("smithy.api#http"), value=MappingProxyType({ | ||
| "method": "GET", | ||
| "uri": "/default-config?grouped=true", | ||
| })), | ||
| Trait.new(id=ShapeID("smithy.api#readonly")), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Smithy @http trait uri query string parameters best practice
💡 Result:
In Smithy, the best practice is: keep @http(uri: ...) for the path template only, and model query strings using HTTP binding traits on input members.
Recommended patterns
1) Put path params in uri using labels, and bind them with @httpLabel
@http(uri: "/things/{thingId}")defines the path pattern.- The
urimust start with/and must not end with?. (smithy.io)
2) Model explicit query parameters with @httpQuery("name")
Use this when you know the query keys ahead of time (typed, documented, stable API). Example: color, shape, size. (smithy.io)
3) Use @httpQueryParams only for “bag of query params” maps
Use this when you need arbitrary query keys (e.g., filters/tags), by binding a map<string, string> (or map<string, list<string>>) to the query string. Notes:
- Only one member in the input can be
@httpQueryParams. - It’s only considered on top-level input members of an operation.
- Avoid mixing
@httpQueryParamswith other@httpQuerymembers unless you’re sure there’s no key overlap (Smithy even warns this can conflict). (smithy.io)
4) Avoid encoding dynamic query params directly in @http(uri: "...?...")
Smithy allows literal query-string requirements in the uri pattern (e.g., “this key must be present”), but the query portion must not contain labels, so it’s not the right tool for parameter binding. Prefer @httpQuery / @httpQueryParams for actual parameters. (smithy.io)
Quick rule of thumb
- Stable, documented query keys →
@httpQuery - Arbitrary/dynamic query keys →
@httpQueryParams - Don’t try to “template” query params inside
@http(uri: ...)(labels aren’t allowed there). (smithy.io)
Citations:
- 1: https://smithy.io/2.0/spec/http-bindings.html?utm_source=openai
- 2: https://smithy.io/2.0/spec/http-bindings.html?utm_source=openai
- 3: https://smithy.io/2.0/spec/http-bindings.html?utm_source=openai
- 4: https://smithy.io/2.0/spec/http-bindings.html?utm_source=openai
- 5: https://smithy.io/2.0/spec/http-bindings.html?utm_source=openai
🌐 Web query:
Smithy httpQuery trait vs embedding query in uri
💡 Result:
In Smithy, you should model query-string parameters using @httpQuery (or @httpQueryParams), not by embedding ?foo={foo} into the @http trait’s uri.
@httpQueryis the standard binding for query parameters. It binds an input structure member to a named query-string parameter and Smithy tooling is expected to handle percent-encoding/decoding automatically. [1]@httpQueryonly applies to top-level members of an operation input (so it’s unambiguous what comes from the query string). [1]@httpLabelis for path segments only, i.e., values substituted into the@http(uri: "/{label}")path—not the query string. [1]- For “arbitrary” query keys/values, use
@httpQueryParamsto bind amapinto the query string. [1]
Practical takeaway: keep @http(uri: ...) as a clean path template, and represent query-string fields with @httpQuery / @httpQueryParams so generators/validators/serializers consistently understand the API shape. [1]
Sources:
[1] Smithy 2.0 Spec – HTTP bindings (traits httpLabel, httpQuery, httpQueryParams, encoding rules, top-level input rules) https://smithy.io/2.0/spec/http-bindings.html
🏁 Script executed:
# Search for Smithy `@http` usage patterns in the codebase
rg "@http|@httpQuery" -A 3 -B 1 | head -100Repository: juspay/superposition
Length of output: 5737
🏁 Script executed:
# Check the file structure and look for other `@http` definitions
cd clients/python/sdk && rg "uri.*\?" -A 2 -B 2 | head -50Repository: juspay/superposition
Length of output: 486
Model the grouped query parameter as an @httpQuery member instead of embedding it in the uri.
Smithy's @http uri is a path template and should not include query strings. Per Smithy specification, query parameters must be bound using @httpQuery on input structure members, which ensures proper serialization, percent-encoding, and API clarity. Update the Smithy model to move grouped to a @httpQuery member, change the uri to /default-config, and regenerate the SDK.
Suggested model update
- "uri": "/default-config?grouped=true",
+ "uri": "/default-config",Then add @httpQuery("grouped") to the input structure member.
🤖 Prompt for AI Agents
In `@clients/python/sdk/superposition_sdk/_private/schemas.py` around lines 7493 -
7504, The LIST_GROUPED_DEFAULT_CONFIGS operation currently embeds the grouped
query param in the smithy.api#http trait's "uri"
("/default-config?grouped=true"); change the http trait value to use uri
"/default-config" and instead add an `@httpQuery`("grouped") member on the
operation's input structure (so that the grouped flag is modeled as a query
member and serialized/percent-encoded correctly), then regenerate the SDK so the
Schema (LIST_GROUPED_DEFAULT_CONFIGS) and generated client code reflect the new
`@httpQuery` binding rather than a query string in the http URI.
| group_entries.sort_by(|(a_key, a_date), (b_key, b_date)| { | ||
| let ord = match (a_date, b_date) { | ||
| (Some(a_dt), Some(b_dt)) => a_dt.cmp(b_dt), | ||
| (None, Some(_)) => Ordering::Less, | ||
| (Some(_), None) => Ordering::Greater, | ||
| (None, None) => a_key.cmp(b_key), | ||
| }; | ||
| apply_sort_order(sort_by, ord) |
There was a problem hiding this comment.
None dates sort before present dates in ascending order — verify this is intentional.
When a group has no leaf configs (so date_for returns None), (None, Some(_)) => Ordering::Less places it before groups that do have dates in ascending sort. This means "unknown date" groups appear first. The more common convention is to sort None last regardless of direction. Confirm this is the desired UX.
🤖 Prompt for AI Agents
In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines
459 - 466, The sort currently treats None dates as Less which places "unknown
date" groups first; change the comparator in group_entries.sort_by (the closure
using a_date, b_date and apply_sort_order) so that None is considered greater
than Some (i.e., sorts last) regardless of ascending/descending; update the
match arms involving (None, Some(_)) and (Some(_), None) to return
Ordering::Greater and Ordering::Less respectively (and keep the tie-breaker on
a_key when both None) so apply_sort_order(sort_by, ord) yields the UX of unknown
dates appearing after known dates.
| if let Some(ref config_names) = filters.name { | ||
| builder = builder.filter(dsl::key.eq_any(config_names.0.clone())); | ||
| } else if let Some(ref search) = filters.search { | ||
| let pattern = format!("%{}%", search); | ||
| builder = builder.filter(dsl::key.ilike(pattern)); | ||
| } |
There was a problem hiding this comment.
LIKE wildcard characters in search are not escaped.
If a user passes % or _ in the search string, these are LIKE pattern wildcards and will produce unintended matches. For example, searching for a_b would match a.b, axb, etc.
🛡️ Proposed fix to escape LIKE metacharacters
} else if let Some(ref search) = filters.search {
- let pattern = format!("%{}%", search);
+ let escaped = search.replace('\\', "\\\\").replace('%', "\\%").replace('_', "\\_");
+ let pattern = format!("%{}%", escaped);
builder = builder.filter(dsl::key.ilike(pattern));
}🤖 Prompt for AI Agents
In `@crates/context_aware_config/src/api/default_config/handlers.rs` around lines
643 - 648, The search string used in the ilike pattern isn't escaping SQL LIKE
metacharacters, so '%' or '_' in filters.search will act as wildcards; before
building the pattern in handlers.rs, escape backslashes, '%' and '_' in
filters.search (e.g. replace '\' -> '\\', '%' -> '\%', '_' -> '\_'), then build
the pattern as format!("%{}%", escaped_search) and use that pattern in
builder.filter(dsl::key.ilike(pattern)); ensure the escape sequences are applied
to filters.search (the same variable referenced in the current if-let branch) so
unwanted wildcard matches are prevented.
There was a problem hiding this comment.
suggest a better way to allow substring match only
There was a problem hiding this comment.
@ayushjain17 Here are better approaches for substring-only matching:
Recommended: Escape LIKE Wildcards with a Helper Function
The most practical approach is to escape LIKE metacharacters, but wrap it in a reusable helper:
// Add this helper function near the top of the file or in a utilities module
fn escape_like_pattern(input: &str) -> String {
input
.replace('\\', "\\\\") // Escape backslash first
.replace('%', "\\%") // Escape percent
.replace('_', "\\_") // Escape underscore
}
// Then use it in your query:
} else if let Some(ref search) = filters.search {
let escaped = escape_like_pattern(search);
let pattern = format!("%{}%", escaped);
builder = builder.filter(dsl::key.ilike(pattern));
}Pros:
- Simple and maintainable
- Works with existing Diesel query builder
- Reusable across codebase
- No performance overhead
- Familiar SQL LIKE semantics
Alternative: Use PostgreSQL position() Function
For a wildcard-free approach:
use diesel::dsl::sql;
use diesel::sql_types::Bool;
} else if let Some(ref search) = filters.search {
// Escape single quotes for SQL string literal
let safe_search = search.replace('\'', "''");
let search_lower = safe_search.to_lowercase();
builder = builder.filter(
sql::<Bool>(&format!(
"position('{}' in lower(key)) > 0",
search_lower
))
);
}Pros:
- No wildcard confusion
- Explicit substring search
Cons:
- Requires raw SQL (less type-safe)
- Still need to escape SQL string delimiters (single quotes)
Alternative: Use Diesel's .contains() (if available)
Check if Diesel supports a contains helper for your column type - this would be the cleanest:
} else if let Some(ref search) = filters.search {
builder = builder.filter(dsl::key.contains(search));
}Note: This may not be available for all column types or may require additional traits.
My recommendation: Use the escaped LIKE pattern with a helper function (first option). It's the most maintainable, works well with Diesel's query builder, and clearly communicates intent without requiring raw SQL.
There was a problem hiding this comment.
should I support LIKE wildcards ?
There was a problem hiding this comment.
@ayushjain17 I recommend NOT supporting LIKE wildcards by default. Here's why:
Reasons to Escape Wildcards (Simple Substring Search)
👍 Better for most users:
- Users searching for
a_bexpect to find literallya_b, notaxbora.b - More intuitive behavior (matches Google, VS Code search, etc.)
- No learning curve - just type what you want to find
👍 Safer:
- Prevents accidental broad matches (e.g., searching
a%matching everything starting witha) - Avoids performance issues from unintended leading wildcards
👍 Predictable:
- Config keys are structured (e.g.,
app.feature.setting) - Substring search is sufficient for most use cases
When Wildcards Might Be Useful
- Power users wanting pattern-based searches
- But regex or structured filters (prefix, name array) already provide this
- Adds complexity - users need to know about
%,_, and escaping - Could confuse non-technical users
%at the start of patterns prevents index usage (slow on large datasets)
My Recommendation
Use escaped substring search (option 1 from my previous message):
fn escape_like_pattern(input: &str) -> String {
input
.replace('\\', "\\\\")
.replace('%', "\\%")
.replace('_', "\\_")
}
// In query:
let escaped = escape_like_pattern(search);
let pattern = format!("%{}%", escaped);
builder = builder.filter(dsl::key.ilike(pattern));Why: Simple, safe, and matches user expectations. You already have prefix and name filters for structured searches, so search can focus on simple substring matching.
Future: If power users request wildcard support later, you could add a separate pattern parameter or a flag like enable_wildcards=true.
| let redirect_url = move |prefix: Option<String>| -> String { | ||
| let get_updated_query = use_update_url_query(); | ||
| get_updated_query("prefix", prefix) | ||
| let page_params = pagination_params_rws.get(); | ||
|
|
||
| get_updated_query(&[ | ||
| ("prefix", prefix), | ||
| ("name", None), | ||
| ("page", page_params.page.map(|_| "1".to_string())), | ||
| ("count", page_params.count.map(|c| c.to_string())), | ||
| ("all", page_params.all.map(|a| a.to_string())), | ||
| ]) | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
use_update_url_query() is invoked inside a regular closure, not at component top-level.
In Leptos, use_* hooks are conventionally called during component initialization to ensure they have access to the correct reactive owner/scope. Calling use_update_url_query() lazily inside redirect_url (which is invoked from the view) may work if it only reads router context, but it's fragile and could break if the hook's internals change.
Consider hoisting the call:
♻️ Suggested change
+ let get_updated_query = use_update_url_query();
let redirect_url = move |prefix: Option<String>| -> String {
- let get_updated_query = use_update_url_query();
let page_params = pagination_params_rws.get();
get_updated_query(&[🤖 Prompt for AI Agents
In `@crates/frontend/src/pages/default_config_list.rs` around lines 101 - 112, The
closure redirect_url currently calls use_update_url_query() inside the closure
body which violates Leptos' hook call timing; hoist the call to
use_update_url_query() to component initialization (top-level of the component
function) and store its returned function/handle in a variable (e.g.,
update_query_fn), then have redirect_url capture and call that variable along
with pagination_params_rws.get() and the provided prefix; this ensures
use_update_url_query() is invoked during component setup rather than lazily
inside redirect_url.
| "/default-config?grouped=true": { | ||
| "get": { | ||
| "description": "Retrieves a paginated list of all default config entries in the workspace, including their values, schemas, and metadata.", | ||
| "operationId": "ListGroupedDefaultConfigs", | ||
| "parameters": [ | ||
| { | ||
| "name": "count", | ||
| "in": "query", | ||
| "description": "Number of items to be returned in each page.", | ||
| "schema": { | ||
| "type": "number", | ||
| "description": "Number of items to be returned in each page." | ||
| } | ||
| }, | ||
| { | ||
| "name": "page", | ||
| "in": "query", | ||
| "description": "Page number to retrieve, starting from 1.", | ||
| "schema": { | ||
| "type": "number", | ||
| "description": "Page number to retrieve, starting from 1." | ||
| } | ||
| }, | ||
| { | ||
| "name": "all", | ||
| "in": "query", | ||
| "description": "If true, returns all requested items, ignoring pagination parameters page and count.", | ||
| "schema": { | ||
| "type": "boolean", | ||
| "description": "If true, returns all requested items, ignoring pagination parameters page and count." | ||
| } | ||
| }, | ||
| { | ||
| "name": "name", | ||
| "in": "query", | ||
| "style": "form", | ||
| "schema": { | ||
| "type": "array", | ||
| "items": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "explode": true | ||
| }, | ||
| { | ||
| "name": "prefix", | ||
| "in": "query", | ||
| "schema": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| { | ||
| "name": "sort_by", | ||
| "in": "query", | ||
| "description": "Sort order enumeration for list operations.", | ||
| "schema": { | ||
| "$ref": "#/components/schemas/SortBy" | ||
| } | ||
| }, | ||
| { | ||
| "name": "sort_on", | ||
| "in": "query", | ||
| "schema": { | ||
| "$ref": "#/components/schemas/DefaultConfigSortOn" | ||
| } | ||
| }, | ||
| { | ||
| "name": "x-org-id", | ||
| "in": "header", | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "required": true | ||
| }, | ||
| { | ||
| "name": "x-workspace", | ||
| "in": "header", | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "required": true | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "ListGroupedDefaultConfigs 200 response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/ListGroupedDefaultConfigsResponseContent" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "500": { | ||
| "description": "InternalServerError 500 response", | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/InternalServerErrorResponseContent" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "tags": [ | ||
| "Default Configuration" | ||
| ] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Identify OpenAPI paths that include a query string (invalid per OpenAPI).
python - <<'PY'
import json
from pathlib import Path
spec = json.loads(Path("docs/docs/api/Superposition.openapi.json").read_text())
bad = [p for p in spec.get("paths", {}) if "?" in p]
print("Paths containing '?':", bad)
PYRepository: juspay/superposition
Length of output: 118
OpenAPI paths must not include query strings.
Line 1544 defines "/default-config?grouped=true" as a path key. OpenAPI path items cannot contain a query string, so this spec will fail validation and most SDK generators. Model grouping as a query parameter on /default-config or expose a dedicated path like /default-config/grouped if the backend uses a separate route.
🤖 Prompt for AI Agents
In `@docs/docs/api/Superposition.openapi.json` around lines 1544 - 1652, The
OpenAPI path key "/default-config?grouped=true" is invalid because paths must
not contain query strings; update the spec by moving the grouped flag into a
query parameter on the existing "/default-config" path (add a "grouped" query
parameter with type boolean to the operation with operationId
"ListGroupedDefaultConfigs" and keep its responses/schema referencing
"ListGroupedDefaultConfigsResponseContent"), or alternatively expose a separate
path like "/default-config/grouped" and keep the operationId and responses
as-is; ensure you remove the "?grouped=true" from the path key so the spec
validates.
There was a problem hiding this comment.
smithy based generators worked pretty fine with it, can you please verify the generation for all the languages, I have verified for typescript and haskell
There was a problem hiding this comment.
smithy rust also works fine
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
There was a problem hiding this comment.
even for java as well as python it is fine
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OpenAPI: detect invalid path keys containing '?' =="
python - <<'PY'
import json, sys
from pathlib import Path
p = Path("docs/docs/api/Superposition.openapi.json")
data = json.loads(p.read_text())
bad = [k for k in data.get("paths", {}) if "?" in k]
print("Paths containing '?':", bad)
PY
echo
echo "== Java: presence of operation and wiring =="
rg -nP --glob='clients/java/sdk/**' -C2 'listGroupedDefaultConfigs\s*\(' || true
echo
echo "-- SuperpositionClient.java declarations (context) --"
rg -n --glob='clients/java/sdk/src/main/java/io/juspay/superposition/client/SuperpositionClient.java' -C3 'listGroupedDefaultConfigs' || true
echo
echo "-- Model class exists --"
rg -n --glob='clients/java/sdk/src/main/java/io/juspay/superposition/model/**' 'class ListGroupedDefaultConfigs\b|class ListGroupedDefaultConfigsInput\b|class ListGroupedDefaultConfigsOutput\b' || true
echo
echo "== Python: client method + serde hooks =="
rg -n --glob='clients/python/sdk/**' -C2 'def list_grouped_default_configs\(|_serialize_list_grouped_default_configs|_deserialize_list_grouped_default_configs' || true
echo
echo "== JS/TS: command, client, protocol serde =="
rg -n --glob='clients/javascript/sdk/**' -C2 'class ListGroupedDefaultConfigsCommand|se_ListGroupedDefaultConfigsCommand|de_ListGroupedDefaultConfigsCommand|listGroupedDefaultConfigs\(' || true
echo
echo "== Rust: client method, builders, error type =="
rg -n --glob='crates/superposition_sdk/**' -C2 'pub fn list_grouped_default_configs\(|ListGroupedDefaultConfigsFluentBuilder|ListGroupedDefaultConfigsError' || true
echo
echo "== Haskell: command module + function =="
rg -n --glob='clients/haskell/**' -C2 'module Io\.Superposition\.Command\.ListGroupedDefaultConfigs|listGroupedDefaultConfigs\s*::' || true
echo
echo "== Tests referencing the new command (sanity) =="
rg -n --glob='tests/**' -C2 'ListGroupedDefaultConfigsCommand' || true
Length of output: 46915
@ayushjain17 Thanks — I double‑checked across the repo, and Smithy/Rust (and the other SDKs) look good.
What I verified:
- OpenAPI doc: still has a non‑compliant path key with a query string Paths containing '?': ['/default-config?grouped=true'] (docs/docs/api/Superposition.openapi.json, Line 1544).
- Java: SuperpositionAsyncClient, SuperpositionClient, and SuperpositionClientImpl all expose listGroupedDefaultConfigs(...), and the operation/input/output model classes exist.
- Python: list_grouped_default_configs(...) is wired; serializer builds GET /default-config with grouped=true; deserializer present.
- JS/TS: ListGroupedDefaultConfigsCommand + se_/de_ functions and client overloads present.
- Rust: client method, fluent builder, error type, and serde are in place.
- Haskell: Io.Superposition.Command.ListGroupedDefaultConfigs module exports listGroupedDefaultConfigs.
- Tests: default_config.test.ts exercises ListGroupedDefaultConfigsCommand.
Net: generation is in place for Java, Python, JS/TS, Rust, and Haskell.
Optional doc-only follow-up to make the OpenAPI spec validator-friendly (doesn’t affect Smithy or the SDKs):
- Replace the path key "/default-config?grouped=true" with "/default-config" and add a boolean query param grouped. To keep both shapes in one operation, set 200.response.content.schema.data.items to oneOf [DefaultConfigResponse, GroupedDefaultConfig], or
- If you prefer separate operations in OAS, expose a docs alias like "/default-config/grouped" (only if/when the backend supports it).
Want me to open a small follow-up issue to track the OpenAPI doc fix?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==> OpenAPI paths containing '?' (should be empty):"
python - <<'PY'
import json
from pathlib import Path
p = Path("docs/docs/api/Superposition.openapi.json")
d = json.loads(p.read_text())
bad = [k for k in d.get("paths", {}) if "?" in k]
print(bad)
if "/default-config?grouped=true" in d.get("paths", {}):
op = d["paths"]["/default-config?grouped=true"].get("get", {})
print("operationId at /default-config?grouped=true:", op.get("operationId"))
PY
echo
echo "==> Where is '?grouped=true' referenced?"
rg -n --hidden -S '\?grouped=true' || true
echo
echo "==> Files mentioning 'ListGroupedDefaultConfigs' (count and by extension):"
count=$(rg -l 'ListGroupedDefaultConfigs' | wc -l | tr -d ' ')
echo "Total files: $count"
echo "By extension:"
rg -l 'ListGroupedDefaultConfigs' | awk -F. '{print $NF}' | sed 's/[^a-zA-Z0-9].*$//' | sort | uniq -c | sort -nr
echo
echo "==> Sample files:"
rg -l 'ListGroupedDefaultConfigs' | sed -n '1,20p'
Length of output: 2042
@coderabbit-user Thanks for confirming. I ran a quick repo-wide check:
- Generation is present across languages: 37 files mention ListGroupedDefaultConfigs (9 rs, 8 ts, 7 java, 6 py, 3 hs, plus smithy/mdx/json). Generation “works” in all SDKs you listed.
- The only problematic spot is the OpenAPI doc: docs/docs/api/Superposition.openapi.json defines a path key "/default-config?grouped=true". OpenAPI 3.x forbids query strings in path keys; queries must be modeled as parameters. This will fail standard OpenAPI validators and some doc tooling, even if Smithy-based SDKs generate fine.
Proposed minimal, spec-safe doc fix (no server/code changes):
- Remove the "/default-config?grouped=true" path object.
- Add a boolean grouped query parameter to GET /default-config (and optionally prefix with a note “used when grouped=true”).
- Make the 200 response use oneOf to document both shapes.
Example patch (excerpt):
{
"paths": {
"/default-config": {
"get": {
"parameters": [
{ "name": "count", "in": "query", "schema": { "type": "number" } },
{ "name": "page", "in": "query", "schema": { "type": "number" } },
{ "name": "all", "in": "query", "schema": { "type": "boolean" } },
{ "name": "name", "in": "query", "style": "form", "explode": true,
"schema": { "type": "array", "items": { "type": "string" } } },
{ "name": "sort_by", "in": "query", "schema": { "$ref": "#/components/schemas/SortBy" } },
{ "name": "sort_on", "in": "query", "schema": { "$ref": "#/components/schemas/DefaultConfigSortOn" } },
{ "name": "search", "in": "query", "schema": { "type": "string" } },
{ "name": "grouped", "in": "query",
"description": "If true, returns grouped default configs.",
"schema": { "type": "boolean" } },
{ "name": "prefix", "in": "query",
"description": "Only applicable when grouped=true.",
"schema": { "type": "string" } },
{ "name": "x-org-id", "in": "header", "required": true, "schema": { "type": "string" } },
{ "name": "x-workspace", "in": "header", "required": true, "schema": { "type": "string" } }
],
"responses": {
"200": {
"description": "List default configs (grouped or ungrouped)",
"content": {
"application/json": {
"schema": {
"oneOf": [
{ "$ref": "#/components/schemas/ListDefaultConfigsResponseContent" },
{ "$ref": "#/components/schemas/ListGroupedDefaultConfigsResponseContent" }
]
}
}
}
}
}
}
}
/* remove the "/default-config?grouped=true" entry */
}
}And in docs/docs/api/list-grouped-default-configs.api.mdx, switch the endpoint display to:
- path={"/default-config"}
- Show grouped=true in the request params section.
If you’d like, I can prep a small follow-up to make these doc-only adjustments and run an OpenAPI validation pass. Would you prefer I:
- update this PR with the doc fix, or
- open a separate “Docs: make ListGroupedDefaultConfigs OpenAPI-compliant” issue/PR?
ayushjain17
force-pushed
the
feat/default-config-filter-2
branch
from
dfe2db1 to
e3ff1a1
Compare
1 participant
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit
New Features
Improvements