Skip to content
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

fix(server): addAssets and removeAssets handle duplicate assetIds #9436

Merged
merged 3 commits into from
May 14, 2024

Conversation

eric-barch
Copy link
Contributor

Fixes #9368, where calling addAssets with duplicate ids results in an internal server error.

This fix leverages the existingAssetIds Set already included in addAssets, but I could see the argument that we should keep that reserved for assets that were in the database before calling addAssets. If you feel that way I can instead revise this to create a dedicated Set just for uniqueness within addAssets.

Input:

curl -X 'PUT' \
  'https://demo.immich.app/api/album/c757a1dc-afae-4cce-8f14-cf0adf5bea8e/assets' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "ids": [
    "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "7f7e71ab-a630-449d-b7c1-c52fc8b44733"
  ]
}'

Output:

Before:

{
  "message": "Failed to add assets to album",
  "error": "Internal Server Error",
  "statusCode": 500
}

After:

[
  {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": true
  }, {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": false,
    "error": "duplicate"
  }
]

For consistency, I also made the corresponding change in removeAssets. In my opinion the output from this change is more accurate/helpful, but let me know if you disagree.

Input:

curl -X 'DELETE' \
  'https://demo.immich.app/api/album/c757a1dc-afae-4cce-8f14-cf0adf5bea8e/assets' \
  -H 'accept: application/json' \
  -H 'Content-Type: application/json' \
  -d '{
  "ids": [
    "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "7f7e71ab-a630-449d-b7c1-c52fc8b44733"
  ]
}'

Output:

Before:

[
  {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": true
  }, {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": true
  }
]

After:

[
  {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": true
  }, {
    "id": "7f7e71ab-a630-449d-b7c1-c52fc8b44733",
    "success": false,
    "error": "not_found"
  }
]

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

I'd say this is fine.

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

If you are up for it, can you add an e2e test for this? e2e/src/api/specs/album.e2e-spec.ts

@eric-barch
Copy link
Contributor Author

Sure, no problem. I added two e2e tests, one for each endpoint I changed. I tried to keep the format as close as possible to the existing tests but let me know if you have any suggestions for improvement.

@jrasm91
Copy link
Contributor

jrasm91 commented May 14, 2024

Looks perfect! Tyvm

@jrasm91 jrasm91 enabled auto-merge (squash) May 14, 2024 03:24
@jrasm91 jrasm91 merged commit 6fd6a8b into immich-app:main May 14, 2024
22 checks passed
@eric-barch eric-barch deleted the repeated-ids branch May 14, 2024 03:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addAssetsToAlbum endpoint does not handle repeated IDs
3 participants