Skip to content

Conversation

@AmauryD
Copy link
Member

@AmauryD AmauryD commented Jul 2, 2025

No description provided.

@AmauryD AmauryD requested review from Brenion, Copilot and remadex July 2, 2025 10:15
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR ensures included resources with identical IDs across different resource types are uniquely tracked, adds a test to cover this scenario, and updates CI to newer Node.js and pnpm versions.

  • Use a combined resourceType and ID key in DocumentSerializer to prevent ID collisions in included
  • Add a unit test (serialize-same-ids.test.ts) to verify serialization of resources sharing the same ID
  • Update CI matrix to Node.js 20.x/22.x and bump pnpm to v10

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
serialization/document.ts Use makeUniqueIncludedIdForResource to key included map
tests/unit/.../serialize-same-ids.test.ts New test scenario for same-ID resources
tests/unit/.../snapshots/serialize-same-ids.test.ts.snap Added snapshot for the new test
.github/workflows/test.yml Bumped Node.js matrix to 20.x/22.x and pnpm to v10
Comments suppressed due to low confidence (5)

packages/resource-jsonapi/src/serialization/document.ts:132

  • [nitpick] The helper method name is quite long; consider renaming it to something more concise like getIncludedKey or buildIncludedKey.
  private makeUniqueIncludedIdForResource(r: Resource) {

packages/resource-jsonapi/src/serialization/document.ts:132

  • Add a JSDoc comment explaining the purpose and return format of this helper to aid future maintainers.
  private makeUniqueIncludedIdForResource(r: Resource) {

.github/workflows/test.yml:21

  • [nitpick] Confirm that dropping Node.js 18.x from the CI matrix is intentional and won't impact users still on LTS 18.x.
        node-version: [20.x, 22.x]

.github/workflows/test.yml:33

  • Upgrading to pnpm v10 could introduce breaking changes; ensure all existing workflows and installs remain compatible.
        version: 10

packages/resource-jsonapi/src/serialization/document.ts:132

  • Using _ as a delimiter could collide if resourceType or id contain underscores; consider a delimiter less likely to appear (e.g., :) or encode the parts safely.
  private makeUniqueIncludedIdForResource(r: Resource) {

@AmauryD AmauryD force-pushed the fix-same-id-serialization branch 2 times, most recently from 708a131 to c06505e Compare July 2, 2025 10:23
@AmauryD AmauryD force-pushed the fix-same-id-serialization branch from c06505e to 4979927 Compare July 2, 2025 10:24
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 2, 2025

@AmauryD AmauryD merged commit e5b8c4c into master Jul 2, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants