fix(property-defs-rs API): misc. query and response gen improvements #29332
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR improves the property-defs-rs API with fixes to query generation and response handling.
- Changed JOIN type from INNER to LEFT JOIN for user tables, ensuring property definitions with null user references are still returned
- Added column aliases with prefixes (ub_ for updated_by, vb_ for verified_by) to properly distinguish between user table instances
- Removed unused columns from constants and simplified table column definitions
- Improved URI handling with better fallbacks for scheme and authority in pagination URL generation
- Implemented two-step row deserialization approach to handle the limitations of sqlx with dynamically generated queries
3 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| let raw = row.ub_hedgehog_config.unwrap(); | ||
| hcfg = serde_json::from_str(&raw).unwrap_or(None); | ||
| } |
There was a problem hiding this comment.
logic: This unwrap() could panic if ub_hedgehog_config is None. Since you're already checking with is_some(), consider using unwrap_or_default() or just pass the Option directly to serde_json::from_str.
| let raw = row.ub_hedgehog_config.unwrap(); | |
| hcfg = serde_json::from_str(&raw).unwrap_or(None); | |
| } | |
| if let Some(raw) = &row.ub_hedgehog_config { | |
| hcfg = serde_json::from_str(raw).unwrap_or(None); | |
| } |
| let raw = row.vb_hedgehog_config.unwrap(); | ||
| hcfg = serde_json::from_str(&raw).unwrap_or(None); | ||
| } |
There was a problem hiding this comment.
logic: Same unwrap() issue here as above - could panic if vb_hedgehog_config is None.
| let raw = row.vb_hedgehog_config.unwrap(); | |
| hcfg = serde_json::from_str(&raw).unwrap_or(None); | |
| } | |
| if let Some(raw) = &row.vb_hedgehog_config { | |
| hcfg = serde_json::from_str(raw).unwrap_or(None); | |
| } |
| let mut hcfg: Option<HedgehogConfig> = None; | ||
| if row.ub_hedgehog_config.is_some() { | ||
| let raw = row.ub_hedgehog_config.unwrap(); | ||
| hcfg = serde_json::from_str(&raw).unwrap_or(None); |
There was a problem hiding this comment.
logic: serde_json::from_str returns a Result, not an Option. The unwrap_or(None) will only be used if there's a parsing error, not if the JSON is null or empty.
| let mut hcfg: Option<HedgehogConfig> = None; | ||
| if row.vb_hedgehog_config.is_some() { | ||
| let raw = row.vb_hedgehog_config.unwrap(); | ||
| hcfg = serde_json::from_str(&raw).unwrap_or(None); |
There was a problem hiding this comment.
logic: Same issue with serde_json::from_str here - it returns a Result, not an Option.
Problem
After landing the spike PR for the new
property-defs-rsREST API, I've been testing the API in local dev with a mix of real (hand-clicked like grandma used to make!) and synthetic events. this PR represents the first round of fixes and improvements that shook out.Changes
Of note: I did some research and apparently dynamically generated queries in
sqlxhave a rough spot around deserializing rows into nested structs, even when the struct fields support decoding, so I added an intermediate step. If I'm missing something, let me know 👍 (cc @oliverb123 )Does this work well for both Cloud and self-hosted?
N/A
How did you test this code?
Locally and in CI