-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Preserve state keys #1897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Preserve state keys #1897
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| import { afterEach, describe, expect, it, vi } from 'vitest' | ||
| import { fetchStateItems } from './state' | ||
|
|
||
| function mockStateItems(payload: unknown): void { | ||
| vi.stubGlobal( | ||
| 'fetch', | ||
| vi.fn(async () => ({ | ||
| ok: true, | ||
| json: async () => payload, | ||
| })), | ||
| ) | ||
| } | ||
|
|
||
| describe('fetchStateItems', () => { | ||
| afterEach(() => { | ||
| vi.unstubAllGlobals() | ||
| }) | ||
|
|
||
| it('uses explicit item keys', async () => { | ||
| mockStateItems({ items: [{ key: 'worker', value: { status: 'ready' } }] }) | ||
|
|
||
| const { items } = await fetchStateItems('runtime') | ||
|
|
||
| expect(items[0]).toMatchObject({ | ||
| groupId: 'runtime', | ||
| key: 'worker', | ||
| value: { status: 'ready' }, | ||
| type: 'object', | ||
| }) | ||
| }) | ||
|
|
||
| it('uses map keys for object-shaped item responses', async () => { | ||
| mockStateItems({ items: { 'files/worker/config.yaml': { raw: true } } }) | ||
|
|
||
| const { items } = await fetchStateItems('runtime') | ||
|
|
||
| expect(items[0].key).toBe('files/worker/config.yaml') | ||
| expect(items[0].value).toEqual({ raw: true }) | ||
| }) | ||
|
|
||
| it('uses explicit item keys before generated object entry keys', async () => { | ||
| mockStateItems({ items: { 'item-0': { key: 'worker', value: { status: 'ready' } } } }) | ||
|
|
||
| const { items } = await fetchStateItems('runtime') | ||
|
|
||
| expect(items[0].key).toBe('worker') | ||
| expect(items[0].value).toEqual({ status: 'ready' }) | ||
| }) | ||
|
|
||
| it('uses embedded state_key before falling back', async () => { | ||
| mockStateItems({ items: [{ state_key: 'last_test_suite', status: 'PASS' }] }) | ||
|
|
||
| const { items } = await fetchStateItems('runtime') | ||
|
|
||
| expect(items[0].key).toBe('last_test_suite') | ||
| expect(items[0].value).toEqual({ state_key: 'last_test_suite', status: 'PASS' }) | ||
| }) | ||
|
|
||
| it('does not invent item-N labels when keys are missing', async () => { | ||
| mockStateItems({ items: [{ status: 'unknown' }] }) | ||
|
|
||
| const { items } = await fetchStateItems('runtime') | ||
|
|
||
| expect(items[0].key).toBe('(missing key 0)') | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ use crate::{ | |
| registry::{StateAdapterFuture, StateAdapterRegistration}, | ||
| structs::{ | ||
| StateDeleteInput, StateGetGroupInput, StateGetInput, StateListGroupsInput, | ||
| StateSetInput, StateUpdateInput, | ||
| StateListItem, StateSetInput, StateUpdateInput, | ||
| }, | ||
| }, | ||
| }; | ||
|
|
@@ -38,6 +38,77 @@ impl BridgeAdapter { | |
| } | ||
| } | ||
|
|
||
| fn decode_list_item(item: Value, index: usize) -> StateListItem { | ||
| match item { | ||
| Value::Array(mut tuple) if tuple.len() >= 2 => { | ||
| if let Some(key) = tuple.first().and_then(Value::as_str).map(str::to_string) { | ||
| let value = tuple.swap_remove(1); | ||
| return StateListItem { key, value }; | ||
| } | ||
| StateListItem { | ||
| key: format!("(missing key {index})"), | ||
| value: Value::Array(tuple), | ||
| } | ||
| } | ||
| Value::Object(mut object) => { | ||
| let key = object | ||
| .get("key") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string) | ||
| .or_else(|| { | ||
| object | ||
| .get("state_key") | ||
| .and_then(Value::as_str) | ||
| .map(str::to_string) | ||
| }); | ||
| match key { | ||
| Some(key) => { | ||
| let value = object | ||
| .remove("value") | ||
| .unwrap_or_else(|| Value::Object(object)); | ||
| StateListItem { key, value } | ||
| } | ||
| None => StateListItem { | ||
| key: format!("(missing key {index})"), | ||
| value: Value::Object(object), | ||
| }, | ||
| } | ||
| } | ||
| value => StateListItem { | ||
| key: format!("(missing key {index})"), | ||
| value, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| fn decode_list_result(result: Value) -> anyhow::Result<Vec<StateListItem>> { | ||
| if let Ok(items) = serde_json::from_value::<Vec<StateListItem>>(result.clone()) { | ||
| return Ok(items); | ||
| } | ||
|
|
||
| let result = result.get("items").cloned().unwrap_or(result); | ||
|
|
||
| if let Some(object) = result.as_object() { | ||
| return Ok(object | ||
| .iter() | ||
| .map(|(key, value)| StateListItem { | ||
| key: key.clone(), | ||
| value: value.clone(), | ||
| }) | ||
| .collect()); | ||
|
Comment on lines
+89
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Handle top-level Line 89 unwraps 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| if let Value::Array(items) = result { | ||
| return Ok(items | ||
| .into_iter() | ||
| .enumerate() | ||
| .map(|(index, item)| decode_list_item(item, index)) | ||
| .collect()); | ||
| } | ||
|
|
||
| anyhow::bail!("invalid state::list response: expected array, object, or items field") | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl StateAdapter for BridgeAdapter { | ||
| async fn update( | ||
|
|
@@ -130,7 +201,7 @@ impl StateAdapter for BridgeAdapter { | |
| Ok(()) | ||
| } | ||
|
|
||
| async fn list(&self, scope: &str) -> anyhow::Result<Vec<Value>> { | ||
| async fn list(&self, scope: &str) -> anyhow::Result<Vec<StateListItem>> { | ||
| let data = StateGetGroupInput { | ||
| scope: scope.to_string(), | ||
| }; | ||
|
|
@@ -146,8 +217,7 @@ impl StateAdapter for BridgeAdapter { | |
| .await | ||
| .map_err(|e| anyhow::anyhow!("Failed to list values via bridge: {}", e))?; | ||
|
|
||
| serde_json::from_value::<Vec<Value>>(result) | ||
| .map_err(|e| anyhow::anyhow!("Failed to deserialize list result: {}", e)) | ||
| decode_list_result(result) | ||
| } | ||
|
|
||
| async fn list_groups(&self) -> anyhow::Result<Vec<String>> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unwrap
valueconsistently whenidis used as the key.Line 63 accepts
item.idas a key source, but Line 34 only unwrapsitem.valueforkey/state_key.{ id: 'worker', value: {...} }would render the wrapper object instead of the actual state value.Proposed fix
function stateItemValue(item: Record<string, unknown>): unknown { - if ('value' in item && (nonEmptyString(item.key) || nonEmptyString(item.state_key))) { + if ( + 'value' in item && + (nonEmptyString(item.key) || nonEmptyString(item.state_key) || nonEmptyString(item.id)) + ) { return item.value }📝 Committable suggestion
🤖 Prompt for AI Agents