Skip to content

refactor(crl): use typed exception for CRL lock in progress - #9102

Merged
vitormattos merged 3 commits into
LibreSign:mainfrom
matheusa1:refactor/8117-crl-lock-exception
Oct 21, 2026
Merged

vitormattos merged 3 commits into
LibreSign:mainfrom
matheusa1:refactor/8117-crl-lock-exception

Conversation

@matheusa1

Copy link
Copy Markdown
Contributor

Resolves: part of #8117

📝 Summary

When another process holds the generated-CRL lock, CrlService threw a plain \RuntimeException("CRL %s is already in progress ..."). Both the tests and production code identified that case by its message: CrlRevocationChecker used str_contains($e->getMessage(), 'already in progress') to downgrade it to a debug log.

This PR follows the convention agreed in #8117 (prefer typed exceptions, new public exceptions extend LibresignException):

  • Adds OCA\Libresign\Exception\CrlGenerationInProgressException (extends LibresignException).
  • CrlService throws it instead of \RuntimeException, keeping the same message, so logs are unchanged.
  • CrlRevocationChecker uses instanceof CrlGenerationInProgressException instead of matching the message.
  • CrlServiceTest asserts the exception class instead of expectExceptionMessage().
  • CrlRevocationCheckerTest throws the typed exception, plus a new negative test: an untyped \RuntimeException whose message mentions "already in progress" is now logged as a warning, not treated as a held lock.

All callers of generateCrlDer()/refreshGeneratedCrlDer() catch \Exception/\Throwable, so moving away from \RuntimeException does not change their behavior.

🧪 How to test

composer test:unit -- --filter 'CrlServiceTest|CrlRevocationCheckerTest'

Validation evidence

  • Focused regression/acceptance test: the new negative test and the updated CrlServiceTest cases failed before the change and pass after it.
  • Relevant lint/type/static-analysis checks: php-cs-fixer --dry-run clean on the changed files, php -l OK. Psalm was not run locally (local PHP 8.3.6, Psalm requires >= 8.3.16), so it is left to CI.
  • Broader suite required by the changed surface: --filter Crl ran 243 tests and all pass except Command\Crl\CleanupTest::testRejectsInvalidPeriodFormat ("negative-period"), which fails the same way on main without this change.
  • CI reviewed

⚙️ API / Back-end changes

  • Internal exception type for the "CRL generation/refresh already in progress" case. No API, route, or response shape changes.
  • Unit tests added or updated

✅ Checklist

  • I have read and followed the contribution guide.
  • The diff does not contain unrelated refactors or generated changes.
  • Commits follow Conventional Commits where applicable.
  • All commits include the required DCO sign-off.

🤖 AI (if applicable)

  • The content of this PR was partially or fully generated using AI

Replace the RuntimeException message match in CrlRevocationChecker with a CrlGenerationInProgressException, and assert the exception class in tests instead of its message.
Refs LibreSign#8117

Signed-off-by: Matheus Andrade <2002matheus@gmail.com>
@matheusa1
matheusa1 requested a review from a team as a code owner October 17, 2026 17:11
@welcome

welcome Bot commented Oct 17, 2026

Copy link
Copy Markdown

Thanks for opening your first pull request in this repository! ✌️

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@matheusa1

Thanks for your contribution!

Can you check this review?

Comment thread tests/php/Unit/Service/Crl/CrlRevocationCheckerTest.php Outdated
Comment thread lib/Exception/CrlGenerationInProgressException.php Outdated
Signed-off-by: Matheus Andrade <2002matheus@gmail.com>
@matheusa1
matheusa1 requested a review from YvesCesar October 17, 2026 17:30

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Many thanks, @matheusa1 !

@vitormattos looks okay to me.
I don't think the errors in the pipelines are related to the PR.

@vitormattos
vitormattos merged commit 0b07cb3 into LibreSign:main Oct 21, 2026
56 of 67 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Hi @matheusa1, your first pull request to LibreSign has been merged. 🎉

Your work is part of LibreSign now, and everyone who signs a document with it benefits from the time and knowledge you shared. Thank you.

You are very welcome to contribute again. The good first issue list is a good place to look next, and you no longer have to wonder how the process works.

If you have a few minutes and feel like it, we would love to hear how this went for you: https://ls.librecode.coop/index.php/645932?lang=en&source=github-first-merged-pr&repository=libresign. It is entirely optional; nothing is expected of you in return for contributing.

Come say hello in the LibreSign community: https://t.me/LibreSign

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.

3 participants