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

PoC: Implemented Repository to REST Exception converters #63

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alongosz
Copy link
Member

Question Answer
JIRA issue n/a
Type improvement
Target Ibexa version v4.6
BC breaks no

Ibexa REST defines a few of its own Exception classes and API Exception wrapper classes. In order for an API Exception to be rendered correctly as a response, it needs to be either wrapped with REST Exception which has its own Visitor or actually have dedicated Visitor. If we look at ./src/bundle/Resources/config/value_object_visitors.yml we will find a mix of those approaches.

Instead of trying to cover both REST and Repository API exceptions with one or two separate visitors, I suggest a new idea: Repository Exception converter which would map a Repository Exception into its REST counterpart.

As an example, ContentFieldValidationExceptionConverter, has been implemented to properly map Repository API ContentFieldValidationException to REST ContentFieldValidationException. This allows the following simplification of REST Controllers:

Before:

public function updateCompanyAction(
    APICompany $company,
    RestCompanyUpdateStruct $companyUpdateStruct
): Company {
    try {
        $updatedCompany = $this->companyService->updateCompany(
            $company,
            $companyUpdateStruct->companyUpdateStruct
        );

        return new RestCompany($updatedCompany);
    } catch (APIContentFieldValidationException $e) {
        throw new RestContentFieldValidationException($e);
    }
}

After:

public function updateCompanyAction(
    APICompany $company,
    RestCompanyUpdateStruct $companyUpdateStruct
): Company {
    $updatedCompany = $this->companyService->updateCompany(
        $company,
        $companyUpdateStruct->companyUpdateStruct
    );

    return new RestCompany($updatedCompany);
}

Paths to explore

  • Maybe introduce more generic extension point that would try to find converter for any non-REST exception, but there might be too much ambiguity to handle that.

TODO

  • Cover all RepositoryExceptions with proper Converters
  • Remove DIC and visitor logic that now would be handled by Converters

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review

@sonarcloud
Copy link

sonarcloud bot commented May 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@adamwojs
Copy link
Member

+1 from conceptual point of view

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.

4 participants