fix parsing bug #579
fix parsing bug #579
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a parsing bug where numeric properties with a value of 0 were being incorrectly filtered out during entity parsing. The fix changes the condition from checking for truthy values (if (rawValue)) to explicitly checking for undefined (if (rawValue !== undefined)), and adds fallback logic to handle cases where the API returns numbers as strings instead of numeric values.
Changes:
- Updated condition checks in entity parsing to use
!== undefinedinstead of truthy checks to properly handle falsy values like 0 - Added fallback logic in
convertPropertyValueto parse numbers from string fields when the number field is null - Added temporary test script and configuration for manual testing
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/hypergraph/src/utils/convert-property-value.ts | Enhanced number property parsing with fallback to string field and explicit undefined handling |
| packages/hypergraph/src/utils/convert-relations.ts | Changed condition to !== undefined for proper handling of falsy numeric values |
| packages/hypergraph/src/entity/find-one-public.ts | Changed condition to !== undefined for proper handling of falsy numeric values |
| packages/hypergraph/src/entity/find-many-public.ts | Changed condition to !== undefined for proper handling of falsy numeric values |
| apps/events/test-script.ts | Added temporary manual test script (should be removed) |
| apps/events/package.json | Added script command for running test script (should be removed if test-script.ts is removed) |
| .claude/settings.local.json | Added permissions for test script execution |
| .changeset/fix-number-property-parsing.md | Added changeset documentation for the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "preview": "vite preview", | ||
| "typesync": "hypergraph typesync" | ||
| "typesync": "hypergraph typesync", | ||
| "test:script": "tsx test-script.ts" |
There was a problem hiding this comment.
This script command appears to be for running a temporary test file. If the test-script.ts file is removed (as it appears to be temporary development code), this script entry should also be removed.
| "Bash(pnpm --filter events test:script:*)", | ||
| "Bash(pnpm test:*)" |
There was a problem hiding this comment.
These permission entries appear to be related to running the temporary test script. If test-script.ts is removed, consider also removing these related permission entries to keep the configuration clean.
| if (propertyType.value === 'number') { | ||
| return Number(property.number); | ||
| // Handle case where number is stored as string in the API | ||
| if (property.number != null) { | ||
| return Number(property.number); | ||
| } | ||
| if (property.string != null) { | ||
| return Number(property.string); | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
The new number parsing logic that handles numbers stored as strings lacks test coverage. Since the repository has comprehensive test coverage for entity parsing (e.g., find-one-public.test.ts, find-many-public.test.ts), consider adding tests to verify: 1) numbers stored in the number field are correctly parsed, 2) numbers stored as strings in the string field are correctly parsed as a fallback, 3) the value 0 is correctly handled (not filtered out), and 4) edge cases like empty strings or invalid numeric strings are handled appropriately.
| if (property.string != null) { | ||
| return Number(property.string); |
There was a problem hiding this comment.
When converting a number property from a string, if the string is empty or contains non-numeric content, this will return NaN (Not a Number). Consider adding validation to check if the conversion results in NaN and either return undefined or throw a more informative error. For example, you could use isNaN() to validate the result before returning it.
| import { SystemIds } from '@graphprotocol/grc-20'; | ||
| import { Config, Entity, Id, Type } from '@graphprotocol/hypergraph'; | ||
|
|
||
| Config.setApiOrigin('https://testnet-api.geobrowser.io'); | ||
|
|
||
| const BOUNTY_TYPE_ID = Id('327976dea5ad45769b83b7e7ec6337cf'); | ||
| const REWARD_PROPERTY_ID = Id('e8e7301136354e84b46b767e7cd530a8'); | ||
|
|
||
| const Bounty = Entity.Schema( | ||
| { | ||
| name: Type.String, | ||
| description: Type.String, | ||
| reward: Type.Number, | ||
| }, | ||
| { | ||
| types: [BOUNTY_TYPE_ID], | ||
| properties: { | ||
| name: SystemIds.NAME_PROPERTY, | ||
| description: SystemIds.DESCRIPTION_PROPERTY, | ||
| reward: REWARD_PROPERTY_ID, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| async function main() { | ||
| const bounty = await Entity.findOnePublic(Bounty, { | ||
| id: '93c9d09e662840a891fefe4c505f9365', | ||
| space: 'b0043a26cb81379c1217dfd2283b67b8', | ||
| }); | ||
| console.log(bounty); | ||
| } | ||
|
|
||
| main(); |
There was a problem hiding this comment.
This test script file appears to be temporary development/debugging code and should not be committed to the repository. It contains hardcoded IDs and API endpoints specific to testing. Consider removing this file or moving it to a proper test directory with appropriate test structure if it's intended to be part of the test suite.
No description provided.