feat: add missing encoder/decoder capabilities + missing tests - #699
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a new simple URL codec package, including encoder and decoder APIs, serializer reset support, serializer visibility changes, relation model simplification, parser robustness for optional schemas, and new unit tests covering fields, pagination, relations, and sorts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
🤖 Fix all issues with AI agents
In `@packages/codec-url-simple/package.json`:
- Around line 2-4: In package.json update the repository.directory field to
match the renamed package by changing its value from the old path
(packages/codec) to the new directory (packages/codec-url-simple); open the
package.json for `@rapiq/codec-url-simple` and modify the "repository.directory"
property so it points to "packages/codec-url-simple" to keep repository metadata
consistent.
In `@packages/codec-url-simple/test/unit/sort.spec.ts`:
- Line 13: The describe block name is wrong: update the test suite declaration
describe('relations', () => { to describe('sort', () => { so the suite name
matches the file and tests for the sort functionality (look for the describe
call in packages/codec-url-simple/test/unit/sort.spec.ts and change the string
literal from 'relations' to 'sort').
In `@packages/parser-simple/src/parameter/relations/module.ts`:
- Around line 60-75: The code currently checks only schema.throwOnFailure when
deciding to throw for an invalid key (in the isPathAllowed branch), which
ignores the parsed-in option throwOnFailure; update the logic so the throw
occurs if either schema.throwOnFailure or the incoming throwOnFailure option is
true (i.e., use a combined condition like schema.throwOnFailure ||
throwOnFailure) when calling RelationsParseError.keyInvalid for key.name; keep
the existing behavior for the later isValidPath branch that already uses
throwOnFailure and reuse isPathAllowed, isValidPath, and
RelationsParseError.keyInvalid to locate where to change.
- Around line 96-109: The nested-relation branch uses isPathAllowed(key,
schema.allowed) which skips validation when schema.allowed is undefined; update
the logic in the block handling schema (around applyMapping and isPathAllowed)
so that if schema.allowed is undefined you call the general isValidPath(key)
check (same behavior as top-level), otherwise use isPathAllowed(key,
schema.allowed); ensure the code throws RelationsParseError.keyPathInvalid(key)
when that validation fails and then proceeds to
this.registry.resolve(schema.name, key) only on success.
🧹 Nitpick comments (5)
packages/codec-url-simple/src/encoder/serializer/array.ts (1)
16-25: Consider clearing the array in place.This avoids an allocation and preserves any internal references to
value.♻️ Proposed tweak
- reset() { - this.value = []; - } + reset() { + this.value.length = 0; + }packages/codec-url-simple/test/unit/pagination.spec.ts (2)
22-30: Mutatingvaluebefore assertion masks encode/decode asymmetry.Line 28 modifies the original
value.offset = 0before comparison. This suggestsdecodePaginationreturns aPaginationwithoffset: 0when onlylimitwas provided, but the originalPagination(50)presumably hadoffset: undefined.If this is intentional behavior (decoder normalizes missing offset to 0), consider documenting it or testing the behavior explicitly. Otherwise, this masks an encode/decode round-trip asymmetry.
Also, using the non-null assertion
encoded!will silently fail if encoding unexpectedly returnsnull. Consider adding an explicit assertion:const encoded = encoder.encodePagination(value); +expect(encoded).not.toBeNull(); const decoded = decoder.decodePagination(encoded!);
22-48: Tests are markedasyncbut contain noawaitoperations.The test functions don't perform any asynchronous operations. Remove
asyncfor clarity.✨ Suggested fix
- it('should encode & decode with limit', async () => { + it('should encode & decode with limit', () => {Apply similarly to other test cases.
packages/codec-url-simple/test/unit/sort.spec.ts (1)
22-54: Same observations as pagination tests: unnecessaryasyncand non-null assertions.Consider removing
asyncsince there are noawaitcalls, and add explicit null checks on encoded values for better test failure messages.packages/codec-url-simple/test/unit/fields.spec.ts (1)
14-14: TODO comment indicates missing operator test coverage.The comment
// todo: operator missingsuggests field operators (include/exclude) are not yet tested.Would you like me to help generate test cases for field operators, or should I open an issue to track this?
| "name": "@rapiq/codec-url-simple", | ||
| "version": "1.0.0", | ||
| "description": "A package containing an url encoder & decoder", | ||
| "description": "A package containing an url encoder & decoder for the simple dialect.", |
There was a problem hiding this comment.
Update repository.directory to match the renamed package.
After renaming to @rapiq/codec-url-simple, the repository directory still points to packages/codec, which can mislead consumers. Please update it to the new path (Line 66).
🛠 Proposed fix
- "directory": "packages/codec"
+ "directory": "packages/codec-url-simple"🤖 Prompt for AI Agents
In `@packages/codec-url-simple/package.json` around lines 2 - 4, In package.json
update the repository.directory field to match the renamed package by changing
its value from the old path (packages/codec) to the new directory
(packages/codec-url-simple); open the package.json for `@rapiq/codec-url-simple`
and modify the "repository.directory" property so it points to
"packages/codec-url-simple" to keep repository metadata consistent.
| if (schema) { | ||
| key.name = applyMapping(key.name, schema.mapping); | ||
|
|
||
| if (!isPathAllowed(key.name, schema.allowed)) { | ||
| if (schema.throwOnFailure) { | ||
| throw RelationsParseError.keyInvalid(key.name); | ||
| } | ||
|
|
||
| continue; | ||
| } else if ( | ||
| typeof schema.allowed === 'undefined' && | ||
| !this.isValidPath(key.name) | ||
| ) { | ||
| if (throwOnFailure) { | ||
| throw RelationsParseError.keyInvalid(key.name); | ||
| } |
There was a problem hiding this comment.
Respect options.throwOnFailure when schema is present.
On Line 64, schema.throwOnFailure bypasses the options.throwOnFailure override computed on Line 36, so callers can’t force throws.
Suggested fix
- if (schema.throwOnFailure) {
+ if (throwOnFailure) {
throw RelationsParseError.keyInvalid(key.name);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (schema) { | |
| key.name = applyMapping(key.name, schema.mapping); | |
| if (!isPathAllowed(key.name, schema.allowed)) { | |
| if (schema.throwOnFailure) { | |
| throw RelationsParseError.keyInvalid(key.name); | |
| } | |
| continue; | |
| } else if ( | |
| typeof schema.allowed === 'undefined' && | |
| !this.isValidPath(key.name) | |
| ) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyInvalid(key.name); | |
| } | |
| if (schema) { | |
| key.name = applyMapping(key.name, schema.mapping); | |
| if (!isPathAllowed(key.name, schema.allowed)) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyInvalid(key.name); | |
| } | |
| continue; | |
| } else if ( | |
| typeof schema.allowed === 'undefined' && | |
| !this.isValidPath(key.name) | |
| ) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyInvalid(key.name); | |
| } |
🤖 Prompt for AI Agents
In `@packages/parser-simple/src/parameter/relations/module.ts` around lines 60 -
75, The code currently checks only schema.throwOnFailure when deciding to throw
for an invalid key (in the isPathAllowed branch), which ignores the parsed-in
option throwOnFailure; update the logic so the throw occurs if either
schema.throwOnFailure or the incoming throwOnFailure option is true (i.e., use a
combined condition like schema.throwOnFailure || throwOnFailure) when calling
RelationsParseError.keyInvalid for key.name; keep the existing behavior for the
later isValidPath branch that already uses throwOnFailure and reuse
isPathAllowed, isValidPath, and RelationsParseError.keyInvalid to locate where
to change.
| if (schema) { | ||
| key = applyMapping(keys[i], schema.mapping); | ||
|
|
||
| continue; | ||
| } | ||
| if (!isPathAllowed(key, schema.allowed)) { | ||
| if (throwOnFailure) { | ||
| throw RelationsParseError.keyPathInvalid(key); | ||
| } | ||
|
|
||
| // todo: also pass options.schema | ||
| const relationSchema = this.registry.resolve(schema.name, key); | ||
| if (!relationSchema) { | ||
| if (throwOnFailure) { | ||
| throw RelationsParseError.keyPathInvalid(key); | ||
| continue; | ||
| } | ||
|
|
||
| continue; | ||
| relationSchema = this.registry.resolve(schema.name, key); | ||
| } else { | ||
| key = keys[i]; |
There was a problem hiding this comment.
Nested relations skip isValidPath when allowed is undefined.
Top-level keys validate with isValidPath for undefined schema.allowed, but nested keys don’t. This creates inconsistent validation for invalid paths.
Suggested fix
if (!isPathAllowed(key, schema.allowed)) {
if (throwOnFailure) {
throw RelationsParseError.keyPathInvalid(key);
}
continue;
}
+
+ if (
+ typeof schema.allowed === 'undefined' &&
+ !this.isValidPath(key)
+ ) {
+ if (throwOnFailure) {
+ throw RelationsParseError.keyPathInvalid(key);
+ }
+
+ continue;
+ }
relationSchema = this.registry.resolve(schema.name, key);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (schema) { | |
| key = applyMapping(keys[i], schema.mapping); | |
| continue; | |
| } | |
| if (!isPathAllowed(key, schema.allowed)) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyPathInvalid(key); | |
| } | |
| // todo: also pass options.schema | |
| const relationSchema = this.registry.resolve(schema.name, key); | |
| if (!relationSchema) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyPathInvalid(key); | |
| continue; | |
| } | |
| continue; | |
| relationSchema = this.registry.resolve(schema.name, key); | |
| } else { | |
| key = keys[i]; | |
| if (schema) { | |
| key = applyMapping(keys[i], schema.mapping); | |
| if (!isPathAllowed(key, schema.allowed)) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyPathInvalid(key); | |
| } | |
| continue; | |
| } | |
| if ( | |
| typeof schema.allowed === 'undefined' && | |
| !this.isValidPath(key) | |
| ) { | |
| if (throwOnFailure) { | |
| throw RelationsParseError.keyPathInvalid(key); | |
| } | |
| continue; | |
| } | |
| relationSchema = this.registry.resolve(schema.name, key); | |
| } else { | |
| key = keys[i]; |
🤖 Prompt for AI Agents
In `@packages/parser-simple/src/parameter/relations/module.ts` around lines 96 -
109, The nested-relation branch uses isPathAllowed(key, schema.allowed) which
skips validation when schema.allowed is undefined; update the logic in the block
handling schema (around applyMapping and isPathAllowed) so that if
schema.allowed is undefined you call the general isValidPath(key) check (same
behavior as top-level), otherwise use isPathAllowed(key, schema.allowed); ensure
the code throws RelationsParseError.keyPathInvalid(key) when that validation
fails and then proceeds to this.registry.resolve(schema.name, key) only on
success.
1 participant
resolves #698
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores