Skip to content

[API] APIP allow overwritten configs to be applied#17623

Merged
GSadee merged 21 commits intoSylius:2.0from
Prometee:apip-merging-configs
Feb 14, 2025
Merged

[API] APIP allow overwritten configs to be applied#17623
GSadee merged 21 commits intoSylius:2.0from
Prometee:apip-merging-configs

Conversation

@Prometee
Copy link
Copy Markdown
Contributor

@Prometee Prometee commented Jan 13, 2025

Q A
Branch? 2.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets api-platform/core#6913
License MIT

Add a failed test to demonstrate that the modification of an existing operation is not working.

Summary by CodeRabbit

  • New Features

    • Added a new API endpoint for creating bar resources.
    • Introduced new command and handler for bar-related operations.
    • Added serialization configurations for BarCommand and BazCommand.
    • New resource configuration for the Bar entity and associated operations.
    • Introduced a new service for managing duplicate operations in resource metadata.
  • Configuration

    • Updated API platform configuration with new resource definitions.
    • Added new service definitions for command handlers.
    • Modified configuration paths and parameters.
  • Tests

    • Enhanced test coverage for API configuration merging.
    • Added new test for endpoint input class overwriting.

@Prometee Prometee requested review from a team as code owners January 13, 2025 17:31
@probot-autolabeler probot-autolabeler bot added the API APIs related issues and PRs. label Jan 13, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Jan 13, 2025

Walkthrough

This pull request introduces a new Bar entity and associated configurations within the Sylius API Bundle. The changes include creating a new entity, command, command handler, and API resource configurations using XML and YAML files. The modifications extend the API platform configuration, add service definitions for message handling, and update test cases to verify the new functionality of endpoint and input class overwriting.

Changes

File Change Summary
src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml New XML resource configuration for Bar entity with POST operation
src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml New YAML resource configuration with POST operation and messenger settings
src/Sylius/Bundle/ApiBundle/Tests/Application/config/config.yaml Added new import, parameter, and API platform mapping path; removed service definition
src/Sylius/Bundle/ApiBundle/Tests/Application/config/doctrine/Bar.orm.xml New Doctrine ORM mapping for Bar entity with id, foo, and bar fields
src/Sylius/Bundle/ApiBundle/Tests/Application/config/services.yaml Added FooHandler, BarHandler, and BazHandler services
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BarCommand.php New command class with properties foo and bar
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php New command handler for BarCommand
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Entity/Bar.php New Bar entity with getter and setter methods
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php Updated test method names and added new test for input class overwriting
src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml New serialization configuration for BarCommand class
src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml New serialization configuration for BazCommand class
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BazCommand.php New command class with property baz
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BazHandler.php New command handler for BazCommand
src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php New class for managing duplicated operations in resource metadata collections
src/Sylius/Bundle/ApiBundle/Resources/config/services.xml New service definition for operation metadata collection factory
composer-require-checker.json Added new entries to the symbol whitelist for ApiPlatform classes

Possibly related PRs

  • Add missing changed InventoryBundle service to UPGRADE-2.0 #17595: This PR addresses the renaming of the service sylius.availability_checker, which is relevant to the changes in the main PR that involve the Bar entity and its associated operations, as both relate to the API service layer in Sylius.
  • [UPMERGE] 1.14 -> 2.0 #17604: This PR includes extensive updates to the configuration and service definitions in Sylius, which may impact the API configurations and operations defined in the main PR, particularly regarding how resources are managed and defined.

Suggested labels

Maintenance, Documentation, Admin

Suggested reviewers

  • GSadee
  • mpysiak
  • kulczy

Poem

🐰 A Rabbit's Ode to Bar and Code 🐰

In Sylius' realm, a new Bar takes flight,
With commands and handlers, shining bright.
XML and YAML, configs unfurl,
A dance of data, a digital swirl.
Code hopping forward, with playful might! 🚀


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a22aba2 and 8595258.

📒 Files selected for processing (1)
  • src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
  • GitHub Check: Static checks / PHP 8.2, Symfony ^6.4

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1)

18-18: Remove unused import

The ShopUserInterface is imported but never used in this class.

-use Sylius\Component\Core\Model\ShopUserInterface;
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php (1)

74-78: Add assertions for response content

The test should verify the response content to ensure the input was properly processed.

     static::createClient()->request('POST', '/api/v2/shop/bar', [
         'json' => [
             'foo' => 'test',
         ],
     ]);

     self::assertResponseIsSuccessful();
+    self::assertJsonContains([
+        '@type' => 'Bar',
+        'foo' => 'test',
+    ]);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e6da24 and 813f7dc.

📒 Files selected for processing (9)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/config.yaml (2 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/doctrine/Bar.orm.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/services.yaml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BarCommand.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Entity/Bar.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php (3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Static checks / PHP 8.3, Symfony ^7.1
  • GitHub Check: Static checks / PHP 8.2, Symfony ^6.4
🔇 Additional comments (6)
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1)

24-27: Handler ignores command input

The handler creates a new Bar instance without using the email and password from the command. This may not effectively demonstrate the API operation modification issue.

Consider using the command properties:

    public function __invoke(BarCommand $command): Bar
    {
-       return new Bar();
+       $bar = new Bar();
+       $bar->setFoo($command->email);
+       return $bar;
    }
✅ Verification successful

Handler should utilize command properties

The review comment is correct. The Bar class has a setFoo method and the BarCommand has an email property. The suggested modification properly demonstrates command handling by utilizing the command's input properties.

🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Find BarCommand class definition
ast-grep --pattern 'class BarCommand'

# Find Bar class definition
ast-grep --pattern 'class Bar'

# Look for any existing patterns of Bar instantiation
rg -A 3 'new Bar\(' 

Length of output: 575


Script:

#!/bin/bash
# Find the specific files in the test application directory
fd -e php Bar src/Sylius/Bundle/ApiBundle/Tests/Application/src/
fd -e php BarCommand src/Sylius/Bundle/ApiBundle/Tests/Application/src/

# If files are found, show their contents
find src/Sylius/Bundle/ApiBundle/Tests/Application/src/ -type f -name "*.php" -exec cat {} \;

Length of output: 32361

src/Sylius/Bundle/ApiBundle/Tests/Application/config/services.yaml (1)

3-9: LGTM! Command handlers are properly configured.

The command handlers are correctly configured with appropriate messenger tags and bus assignments.

src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1)

1-15: Verify resource class and operation alignment

The operation foo_api_shop_bar_post is configured under the customer model (%sylius.model.customer.class%) but seems to be related to the Bar entity based on:

  • Operation name contains "bar"
  • Input class is BarCommand
  • URI template is /shop/bar

This misalignment might be intentional for testing purposes, but please verify if this is the expected configuration.

✅ Verification successful

Resource configuration is valid for testing purposes

The configuration is correctly placed in a test environment (Tests/Application/config/api_platform_modified) and the apparent misalignment between the customer model and Bar operation is intentional for testing purposes. No changes needed.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential resource configuration inconsistencies

# Check if Bar entity is configured elsewhere
rg -l "sylius.model.(bar|Bar).class" 

# Check for other operations using BarCommand
rg -l "BarCommand" --type yaml --type xml

Length of output: 174


Script:

#!/bin/bash
# Check for related test files
rg -l "foo_api_shop_bar" --type php

# Check the directory structure for test context
fd . src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified -t d

Length of output: 208

src/Sylius/Bundle/ApiBundle/Tests/Application/config/doctrine/Bar.orm.xml (1)

14-22: LGTM! Doctrine mapping is properly configured.

The entity mapping follows Doctrine best practices with appropriate field types and generation strategy.

src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml (1)

19-47: ⚠️ Potential issue

Critical: Conflicting API Platform configurations detected

There are inconsistencies between Bar.xml and Bar.yaml configurations for the same operation foo_api_shop_bar_post:

  1. Input class mismatch:

    • XML: Sylius\Bundle\ApiBundle\Command\Account\RegisterShopUser
    • YAML: Sylius\Bundle\ApiBundle\Application\Command\BarCommand
  2. Denormalization group mismatch:

    • XML: sylius:shop:customer:create
    • YAML: sylius:shop:bar:create

This will likely cause issues with API Platform's resource loading.

src/Sylius/Bundle/ApiBundle/Tests/Application/config/config.yaml (1)

23-24: Verify API Platform mapping path configuration

Multiple mapping paths are configured for API Platform:

paths:
    - '%kernel.project_dir%/config/api_platform'
    - '%kernel.project_dir%/config/api_platform_modified'

This could lead to resource configuration conflicts, as seen with the conflicting Bar configurations in both directories.

@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 13, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands:

  • 🚀 /bns:deploy to redeploy the environment

@Prometee Prometee force-pushed the apip-merging-configs branch from ad45f63 to ffa759f Compare January 13, 2025 18:36
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (5)
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BazCommand.php (1)

16-22: Consider adding validation constraints

Since this is a command class that will be used as API input, consider adding validation constraints to ensure data integrity.

 class BazCommand
 {
     public function __construct(
+        #[Assert\NotBlank]
+        #[Assert\Length(min: 1)]
         public readonly string $foo,
     ) {
     }
 }
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BarCommand.php (1)

16-23: Add property documentation and validation

The command properties lack documentation and validation constraints.

+/**
+ * @see Bar
+ */
 class BarCommand
 {
     public function __construct(
+        #[Assert\NotBlank(message: 'sylius.bar.foo.not_blank')]
         public readonly string $foo,
+        #[Assert\NotBlank(message: 'sylius.bar.bar.not_blank')]
         public readonly string $bar,
     ) {
     }
 }
src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1)

2-15: Resource configuration has inconsistent naming and context

Several architectural concerns:

  1. The resource uses %sylius.model.customer.class% but defines a Bar-related endpoint
  2. The endpoint path /shop/bar uses BazCommand as input, mixing Bar and Baz contexts
  3. Missing standard configurations like security, openapi context, etc.

Consider restructuring:

  1. Use the correct resource class
-    '%sylius.model.customer.class%':
+    'Sylius\Bundle\ApiBundle\Application\Entity\Bar':
  1. Align command and endpoint naming
  2. Add missing configurations:
    security: "is_granted('ROLE_API_ACCESS')"
    openapi:
        summary: 'Creates a new Bar resource'
src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (1)

20-22: Fix indentation in serialization groups.

The indentation for group elements is inconsistent. Use spaces instead of tabs for consistency with Symfony standards.

-			<group>sylius:shop:bar:create</group>
+            <group>sylius:shop:bar:create</group>
src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml (1)

39-46: Consider using a more specific validation group.

The validation group 'sylius' seems too generic. Consider using a more specific group like 'sylius:shop:bar:create' to match the denormalization context.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 813f7dc and ffa759f.

📒 Files selected for processing (11)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/services.yaml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BarCommand.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BazCommand.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BazHandler.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Entity/Bar.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Entity/Bar.php
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/services.yaml
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Packages / Get matrix
  • GitHub Check: Frontend / Get matrix
  • GitHub Check: End-to-end tests (MySQL) / Get matrix
  • GitHub Check: End-to-end tests (MariaDB) / Get matrix
🔇 Additional comments (4)
src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1)

4-9: Verify operation configuration

The operation uses messenger but doesn't specify status codes or error responses.

Add standard API Platform operation configurations:

     ApiPlatform\Metadata\Post:
         name: foo_api_shop_bar_post
         input: Sylius\Bundle\ApiBundle\Application\Command\BazCommand
         uriTemplate: '/shop/bar'
         messenger: input
         output: false
+        status: 201
+        exceptionToStatus: 
+            ApiPlatform\Exception\ValidationException: 422
src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (1)

20-23: Verify if 'id' attribute is necessary for BarCommand.

Command objects typically don't require an 'id' attribute since they represent actions rather than entities. Consider removing this attribute unless it serves a specific purpose in your test case.

src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform/resources/Bar.xml (2)

21-28: Clarify the test case purpose.

Since this PR aims to demonstrate an issue with modifying operations, please add a comment explaining:

  1. What specific issue this configuration is meant to demonstrate
  2. How this configuration triggers the issue
  3. What is the expected vs actual behavior

This will help maintainers understand the test case's purpose and expected failure mode.


19-49: Verify potential configuration conflict with YAML definition.

According to the AI summary, there's a YAML configuration file that defines the same operation with a different input type (BazCommand). This might lead to configuration conflicts.

@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (7)
src/Sylius/Bundle/ApiBundle/Tests/Application/config/doctrine/Bar.orm.xml (1)

20-21: Add length constraints to string columns.

The string columns foo and bar are defined without length constraints. This could lead to unexpected behavior or performance issues depending on the database engine.

-        <field name="foo" column="foo" type="string" />
-        <field name="bar" column="bar" type="string" />
+        <field name="foo" column="foo" type="string" length="255" />
+        <field name="bar" column="bar" type="string" length="255" />
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1)

26-27: Encapsulate command properties.

Direct property access on the command object violates encapsulation. Consider using getter methods.

-        $bar->setFoo($command->foo);
-        $bar->setBar($command->bar);
+        $bar->setFoo($command->getFoo());
+        $bar->setBar($command->getBar());
src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BazHandler.php (1)

23-29: Consider adding input validation and error handling.

The handler implementation could be improved in several areas:

  1. No validation of the input command's baz value
  2. No error handling for Bar entity creation
  3. Property naming mismatch (setting 'foo' with 'baz' value) could be confusing
  4. Created entity is not persisted

Consider adding validation and improving error handling:

 public function __invoke(BazCommand $command): Bar
 {
+    if (empty($command->baz)) {
+        throw new \InvalidArgumentException('Baz value cannot be empty');
+    }
+
     $bar = new Bar();
     $bar->setFoo($command->baz);
 
     return $bar;
 }
src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml (1)

21-21: Fix inconsistent indentation.

The indentation uses tabs instead of spaces, which is inconsistent with Symfony's coding standards.

-			<group>sylius:shop:baz:create</group>
+            <group>sylius:shop:baz:create</group>

Also applies to: 25-25

src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (2)

20-30: Consider grouping granularity for attributes.

All attributes are assigned to the same serialization group sylius:shop:bar:create. Consider if more granular grouping would be beneficial for different API operations (e.g., separate groups for create/update operations).


21-21: Fix indentation inconsistency.

The group elements use tabs instead of spaces, which is inconsistent with Symfony's coding standards.

Apply this formatting fix:

-			<group>sylius:shop:bar:create</group>
+            <group>sylius:shop:bar:create</group>

Also applies to: 25-25, 29-29

src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php (1)

35-54: Refactor to eliminate code duplication between operations and GraphQL operations

The blocks handling operations and graphQlOperations in your create method are nearly identical. Refactoring this code can reduce duplication and improve maintainability.

You can extract a private method to handle both cases:

private function transformResourceMetadata(
    ResourceMetadata $resourceMetadata,
    ResourceMetadataCollection $resourceMetadataCollection,
    array &$duplicatedOperationNames
): ResourceMetadata {
    if ($resourceMetadata->getOperations()) {
        $resourceMetadata = $resourceMetadata->withOperations(
            $this->getTransformedOperations(
                $resourceMetadata->getOperations(),
                $resourceMetadataCollection,
                $duplicatedOperationNames,
            ),
        );
    }

    if ($resourceMetadata->getGraphQlOperations()) {
        $resourceMetadata = $resourceMetadata->withGraphQlOperations(
            $this->getTransformedOperations(
                $resourceMetadata->getGraphQlOperations(),
                $resourceMetadataCollection,
                $duplicatedOperationNames,
            ),
        );
    }

    return $resourceMetadata;
}

Then, update your create method:

foreach ($resourceMetadataCollection as $key => $resourceMetadata) {
    $resourceMetadataCollection[$key] = $this->transformResourceMetadata(
        $resourceMetadata,
        $resourceMetadataCollection,
        $duplicatedOperationNames
    );
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffa759f and 11f7c07.

📒 Files selected for processing (11)
  • src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Resources/config/services.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/doctrine/Bar.orm.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BazCommand.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BarHandler.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/CommandHandler/BazHandler.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Entity/Bar.php (1 hunks)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/Sylius/Bundle/ApiBundle/Tests/Application/config/api_platform_modified/resources/Bar.yaml
  • src/Sylius/Bundle/ApiBundle/Tests/Application/src/Tests/MergingConfigsTest.php
🔇 Additional comments (5)
src/Sylius/Bundle/ApiBundle/Tests/Application/src/Command/BazCommand.php (1)

16-22: LGTM! Clean implementation of the command class.

The command follows best practices with:

  • Proper use of readonly property
  • Minimal and focused implementation
  • Correct namespace for test application

Let's verify test coverage for this command:

✅ Verification successful

No dedicated test coverage needed - this is test fixture code

This command is part of the test application fixtures used to test the API Bundle functionality. It's properly integrated with:

  • A command handler (BazHandler)
  • API Platform configuration
  • Serialization mapping

Being test fixture code itself, it doesn't require dedicated test coverage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for tests covering BazCommand
rg -l "class.*Test.*" | xargs rg "BazCommand"

Length of output: 45


Script:

#!/bin/bash
# First find all occurrences of BazCommand
echo "=== Searching for BazCommand usage ==="
rg "BazCommand" -l

echo -e "\n=== Searching for test files in same directory ==="
fd -p "src/Sylius/Bundle/ApiBundle/Tests/Application/src/.*Test\.php$"

echo -e "\n=== Searching for command handlers ==="
rg "BazCommand" -A 5 -B 5

Length of output: 7178

src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BazCommand.xml (1)

1-28: Verify alignment with PR objectives.

The PR's objective was to add a failed test case to illustrate an issue with modifying existing operations. However, the implementation introduces new functionality (BazCommand, handler, and serialization). Could you clarify if this additional implementation is necessary for reproducing the issue?

src/Sylius/Bundle/ApiBundle/Tests/Application/config/serialization/BarCommand.xml (1)

1-18: LGTM! Well-structured XML configuration.

The file header includes proper XML declaration, copyright notice, and correct schema declarations for Symfony's serializer mapping.

src/Sylius/Bundle/ApiBundle/ApiPlatform/Metadata/Resource/Factory/DuplicateOperationReplacerResourceMetadataCollectionFactory.php (1)

1-107: Overall implementation looks good

The implementation effectively handles duplicated operations within the resource metadata collections and enhances the functionality as intended.

src/Sylius/Bundle/ApiBundle/Resources/config/services.xml (1)

105-112: LGTM

The new service definition correctly decorates the existing api_platform.metadata.resource.metadata_collection_factory service with the appropriate priority and configurations.

Comment on lines +73 to +89
foreach ($operations as $name => $operation) {
if (isset($duplicatedOperationNames[$name])) {
continue;
}

$duplicatedOperationNames[$name] = true;

foreach ($this->findOperations($name, $resourceMetadataCollection) as $duplicatedOperation) {
if ($operations instanceof Operations) {
$operations->add($name, $duplicatedOperation);

continue;
}

$operations[$name] = $duplicatedOperation;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid modifying the operations collection during iteration

Modifying the $operations collection while iterating over it can lead to unexpected behavior or skipped elements. It's safer to collect changes separately and apply them after the iteration.

Refactor the code to prevent modifying the collection during iteration:

$additionalOperations = $operations instanceof Operations ? new Operations() : [];

foreach ($operations as $name => $operation) {
    if (isset($duplicatedOperationNames[$name])) {
        continue;
    }

    $duplicatedOperationNames[$name] = true;

    foreach ($this->findOperations($name, $resourceMetadataCollection) as $duplicatedOperation) {
        if ($additionalOperations instanceof Operations) {
            $additionalOperations->add($name, $duplicatedOperation);
        } else {
            $additionalOperations[$name] = $duplicatedOperation;
        }
    }
}

if ($operations instanceof Operations) {
    $operations->addAll($additionalOperations);
} else {
    $operations = array_merge($operations, $additionalOperations);
}

@Prometee Prometee changed the title [API] APIP merging configs [API] APIP allow overwritten configs to be applied Jan 13, 2025
@Sylius Sylius deleted a comment from coderabbitai bot Jan 13, 2025
) {
}

public function create(string $resourceClass): ResourceMetadataCollection
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.

Could you describe a little bit about what exactly you are doing here that solves the problem, ideally by example?

Copy link
Copy Markdown
Contributor Author

@Prometee Prometee Jan 14, 2025

Choose a reason for hiding this comment

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

@GSadee:
This method is creating a ResourceMetadataCollection containing all different Api resources metadata comming from various sources like XML, YAML or PHP attributes.
In the case of Sylius, the returned data is basically an iterable which contains 2 Api resources:

  1. The admin api resource of the current $resourceClass.
  2. The shop api resource of the current $resourceClass.

When a custom operation is overriding a Sylius one then a third Api resource is available:

  1. The custom api resource of the current $resourceClass.

Inside each Api resources we can find various metadata including the list of operations.

Here I iterate over those operations and try to found if the operations is already defined in an other Api resource and replace the current operation with the customized one.

Exemple:

Before this service:

[
    [ // Original api resource
        ...metadata...
        'operations' => ['sylius_admin_bar_post', ['input' => BarCommand::class]],
    ],
    [ // Overrided api resource
        ...metadata...
        'operations' => ['sylius_admin_bar_post', ['input' => BazCommand::class]],
    ],
]

After:

[
    [ // Original api resource
        ...metadata...
        'operations' => ['sylius_admin_bar_post', ['input' => BazCommand::class]],
    ],
    [ // Overrided api resource
        ...metadata...
        'operations' => ['sylius_admin_bar_post', ['input' => BazCommand::class]],
    ],
]

Prometee and others added 2 commits January 14, 2025 10:42
Co-authored-by: Grzegorz Sadowski <sadowskigp@gmail.com>
<argument>%sylius_api.operations_to_remove%</argument>
</service>

<service id="sylius_api.api_platform.metadata.resource.metadata_collection_factory.merger"
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.

Suggested change
<service id="sylius_api.api_platform.metadata.resource.metadata_collection_factory.merger"
<service id="sylius_api.api_platform.metadata.resource.metadata_collection_factory.duplicate_operation_replacer"

@GSadee GSadee merged commit 6c24de6 into Sylius:2.0 Feb 14, 2025
@GSadee
Copy link
Copy Markdown
Member

GSadee commented Feb 14, 2025

Thank you Francis! 🎉

@Prometee Prometee deleted the apip-merging-configs branch February 14, 2025 14:01
GSadee added a commit that referenced this pull request Feb 17, 2025
| Q               | A
|-----------------|-----
| Branch?         | 2.0
| Bug fix?        | no
| New feature?    | no
| BC breaks?      | no
| Deprecations?   | no
| Related tickets | #17623 
| License         | MIT
@coderabbitai coderabbitai bot mentioned this pull request Feb 18, 2025
@coderabbitai coderabbitai bot mentioned this pull request Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

API APIs related issues and PRs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants