Skip to content

feat: add no NAS failure reason and refactor Synology extension exception handling#1480

Merged
mengyushen merged 4 commits into
dtinit:masterfrom
SynologyOpenSource:synology-throw-exception-with-failure-reason
Feb 5, 2026
Merged

feat: add no NAS failure reason and refactor Synology extension exception handling#1480
mengyushen merged 4 commits into
dtinit:masterfrom
SynologyOpenSource:synology-throw-exception-with-failure-reason

Conversation

@simonxander
Copy link
Copy Markdown
Collaborator

@simonxander simonxander commented Jan 28, 2026

Changes:

Core SPI Updates

  • New Failure Reason: Added NO_NAS_IN_ACCOUNT to FailureReasons enum.
  • New Exception: Created NoNasInAccountException in the SPI to handle cases where the destination account lacks a
    NAS setup.

Synology Extension Refactoring

  • Exception Standardization: Removed custom Synology exception classes (e.g., SynologyException,
    SynologyImportException, SynologyErrorCode) and replaced them with standard SPI exceptions:
    • SynologyImportException -> UploadErrorException for general errors
  • Enhanced Error Handling:
    • HTTP 413: Now throws DestinationMemoryFullException.
    • HTTP 422: Checks specific Synology error codes (2000, 2001) and throws NoNasInAccountException.
  • Code Cleanup: Updated SynologyMediaImporter, SynologyPhotosImporter, and SynologyVideosImporter to directly re-throw exceptions for the framework to handle, rather than catching them and returning failure results.
  • Avoid using lambda function for checked exception
    • Replace forEach() by for-loop
    • Replace atomic readyAlbumMap.compute() by synchronized lock protected map operations

Testing

  • Updated all Synology unit tests to assert against the new SPI exceptions.
  • Added specific test cases to verify handling of HTTP 413 and 422 response codes.

Copy link
Copy Markdown
Collaborator

@mengyushen mengyushen left a comment

Choose a reason for hiding this comment

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

It looks like the standard exception classes already cover the scenarios we are handling. Would you mind replacing SynologyException with standard exceptions?

It would be great to avoid introducing custom exceptions if we don't strictly need them.

@simonxander simonxander force-pushed the synology-throw-exception-with-failure-reason branch from 8db09f6 to a46bd3e Compare February 4, 2026 02:56
@simonxander simonxander changed the title feat: map Synology errors to FailureReasons feat: add no NAS failure reason and refactor Synology extension exception handling Feb 4, 2026
Replaced Synology-specific exceptions (SynologyException and subclasses)
with standard SPI exceptions (CopyExceptionWithFailureReason and subclasses)
to align with the core framework and improve error reporting.

Key changes:
- Removed custom Synology exception classes and error codes.
- Added handling for HTTP 413 (Payload Too Large) -> DestinationMemoryFullException.
- Added handling for HTTP 422 (Unprocessable Entity) -> NoNasInAccountException
- Updated Importers (Media, Photos, Videos) to re-throw exceptions instead of catching and returning failure results.
…or handling

Key changes:
- Updated exception assertions to use standard SPI exceptions.
- Added SynologyDTPService tests for HTTP 413 and 422 scenarios.
- Updated method signatures to include throws CopyExceptionWithFailureReason where necessary.
@simonxander simonxander force-pushed the synology-throw-exception-with-failure-reason branch from a46bd3e to 6a6f08d Compare February 4, 2026 03:22
@simonxander simonxander assigned mengyushen and unassigned simonxander Feb 4, 2026
@mengyushen mengyushen assigned simonxander and unassigned mengyushen Feb 4, 2026
Copy link
Copy Markdown
Collaborator

@mengyushen mengyushen left a comment

Choose a reason for hiding this comment

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

Thanks for making the switch to standard exceptions. I appreciate the update!

@mengyushen mengyushen merged commit 2284466 into dtinit:master Feb 5, 2026
6 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