feat: merge items and locations into "entities"#1414
feat: merge items and locations into "entities"#1414tankerkiller125 wants to merge 1 commit intomainfrom
Conversation
This is a massive change that merges the locations and items into entities, allowing them to share structures like custom fields, attachments, etc. BREAKING CHANGE: Backend /v1/items* and /v1/locations* have been entirely replaced by /v1/entities*
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| items, err := ctrl.repo.Items.QueryByAssetID(r.Context(), ctx.GID, repo.AssetID(assetID), int(page), int(pageSize)) | ||
| items, err := ctrl.repo.Entities.QueryByAssetID(r.Context(), ctx.GID, repo.AssetID(assetID), int(page), int(pageSize)) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 12 hours ago
General approach: ensure that the bit size used when parsing matches the bit size of the target type, or add explicit upper-/lower-bound checks before converting to a smaller type. Since we cannot change repo.AssetID or see its exact definition, we should parse assetIDParam directly into the same effective bit width that repo.AssetID uses. CodeQL’s complaint is that we parse as 64 bits then cast to something smaller, so the minimal, non-breaking fix is to reduce the parse bit size to match int (32 bits on CodeQL’s model) while leaving business logic intact.
Best specific fix here: change the assetID parsing on line 33 from ParseInt(..., 10, 64) to ParseInt(..., 10, 32) (matching how page and pageSize are parsed) and adjust assetID’s type to int64 or keep it but note that ParseInt with bitSize=32 already enforces 32-bit bounds. That way, any out-of-range value for a 32-bit type results in a parse error instead of silent truncation when converting to repo.AssetID. The rest of the handler logic and signatures remain unchanged.
Concretely:
- In
backend/app/api/handlers/v1/v1_ctrl_assets.go:- Update the comment and the call on line 33 to use bit size 32:
strconv.ParseInt(assetIDParam, 10, 32). - Keep the error handling and the use of
repo.AssetID(assetID)as is.
- Update the comment and the call on line 33 to use bit size 32:
- No new imports or helper methods are needed.
| @@ -29,8 +29,8 @@ | ||
| ctx := services.NewContext(r.Context()) | ||
| assetIDParam := chi.URLParam(r, "id") | ||
| assetIDParam = strings.ReplaceAll(assetIDParam, "-", "") // Remove dashes | ||
| // Convert the asset ID to an int64 | ||
| assetID, err := strconv.ParseInt(assetIDParam, 10, 64) | ||
| // Convert the asset ID to an int32-sized value; reject out-of-range values | ||
| assetID, err := strconv.ParseInt(assetIDParam, 10, 32) | ||
| if err != nil { | ||
| return err | ||
| } |
|
|
||
| auth := services.NewContext(r.Context()) | ||
| item, err := ctrl.repo.Items.QueryByAssetID(auth, auth.GID, repo.AssetID(assetID), 0, 1) | ||
| item, err := ctrl.repo.Entities.QueryByAssetID(auth, auth.GID, repo.AssetID(assetID), 0, 1) |
Check failure
Code scanning / CodeQL
Incorrect conversion between integer types High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 12 hours ago
In general, the fix is to ensure that when you parse an integer with a given bit size, you do not then convert it to a smaller integer type without first checking that the value lies within that smaller type’s valid range. For signed integer types, this means checking against both the minimum and maximum allowed values. If the value is out of range, you should reject it or handle it gracefully (e.g., return a 400 Bad Request).
For this specific case, we should perform bounds checking on assetID before converting it to repo.AssetID. Since we cannot see repo.AssetID’s exact definition, the safest non-breaking change is to:
- Keep using
strconv.ParseInt(assetIDParam, 10, 64)to allow parsing large values without overflow during parsing. - Add a check that
assetIDis within the representable range of the underlying type ofrepo.AssetID. Because we cannot import or inspect that type here, we’ll conservatively enforce thatassetIDmust be non-negative and fit within a 32-bit signed integer range (0 <= assetID && assetID <= math.MaxInt32), which is a typical size for DB-like IDs and will avoid narrowing from 64 to a smaller int. If it falls outside this range, we’ll return a request error with HTTP 400. - After the bounds check, safely cast to
repo.AssetID(assetID).
To implement this, we need to:
- Add an import for
mathinbackend/app/api/handlers/v1/v1_ctrl_labelmaker.go. - Add a small bounds-check block after parsing
assetIDand before callingctrl.repo.Entities.QueryByAssetID.
| @@ -2,6 +2,7 @@ | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "math" | ||
| "net/http" | ||
| "strconv" | ||
| "strings" | ||
| @@ -114,6 +115,11 @@ | ||
| return err | ||
| } | ||
|
|
||
| // Ensure the parsed ID fits into the underlying type of repo.AssetID | ||
| if assetID < 0 || assetID > math.MaxInt32 { | ||
| return validate.NewRequestError(fmt.Errorf("invalid asset id"), http.StatusBadRequest) | ||
| } | ||
|
|
||
| auth := services.NewContext(r.Context()) | ||
| item, err := ctrl.repo.Entities.QueryByAssetID(auth, auth.GID, repo.AssetID(assetID), 0, 1) | ||
| if err != nil { |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| } | ||
|
|
||
| item, err = svc.repo.Items.UpdateByGroup(ctx, gid, updateItem) | ||
| entity, err = svc.repo.Entities.UpdateByGroup(ctx, gid, updateEntity) |
2 participants
This is a massive change that merges the locations and items into entities, allowing them to share structures like custom fields, attachments, etc.
BREAKING CHANGE: Backend /v1/items* and /v1/locations* have been entirely replaced by /v1/entities*
What type of PR is this?
What this PR does / why we need it:
Merges locations and items into "entities" allowing locations to gain all the functionality of items when it comes to custom fields, attachments, etc.
Which issue(s) this PR fixes:
Fixes: #251
Special notes for your reviewer:
This is a huge change, test everything, and validate everything works and functions correctly