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

[PLA-1735] Adds skipValidation to blockchain transactions #42

Merged
merged 1 commit into from
Apr 21, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Apr 20, 2024

Type

enhancement


Description

  • Added the HasSkippableRules trait to various mutations to support optional validation based on a new skipValidation field.
  • Introduced new methods in mutations to handle validation rules with and without database checks.
  • Updated GraphQL mutation arguments to include the skipValidation field.
  • Added unit tests to ensure the new skip validation functionality works as expected across different mutations.

Changes walkthrough

Relevant files
Enhancement
5 files
CancelListingMutation.php
Integrate Skip Validation Feature in CancelListingMutation

src/GraphQL/Mutations/CancelListingMutation.php

  • Added HasSkippableRules trait to support skipping validation rules.
  • Introduced rulesWithValidation and rulesWithoutValidation methods to
    differentiate validation scenarios.
  • Added getSkipValidationField to the mutation arguments.
  • +21/-2   
    CreateListingMutation.php
    Enhance CreateListingMutation with Conditional Validation

    src/GraphQL/Mutations/CreateListingMutation.php

  • Added HasSkippableRules trait to support conditional validation.
  • Modified validation rules to support skipping under certain
    conditions.
  • Added getSkipValidationField to the mutation arguments.
  • +68/-14 
    FillListingMutation.php
    Add Skip Validation Support to FillListingMutation             

    src/GraphQL/Mutations/FillListingMutation.php

  • Integrated HasSkippableRules trait for optional validation.
  • Added methods for validation with and without database rules.
  • Included getSkipValidationField in mutation arguments.
  • +33/-5   
    FinalizeAuctionMutation.php
    Implement Skip Validation in FinalizeAuctionMutation         

    src/GraphQL/Mutations/FinalizeAuctionMutation.php

  • Added HasSkippableRules trait to allow skipping validation.
  • Created separate methods for handling validation with and without
    database checks.
  • Included getSkipValidationField in mutation arguments.
  • +21/-2   
    PlaceBidMutation.php
    Support Optional Validation in PlaceBidMutation                   

    src/GraphQL/Mutations/PlaceBidMutation.php

  • Implemented HasSkippableRules trait for optional validation.
  • Distinguished between validation rules with and without database
    interaction.
  • Added getSkipValidationField to mutation arguments.
  • +26/-2   
    Tests
    5 files
    CancelListingTest.php
    Test Skip Validation for CancelListingMutation                     

    tests/Feature/GraphQL/Mutations/CancelListingTest.php

  • Added a test case to verify functionality when validation is skipped.
  • +12/-0   
    CreateListingTest.php
    Test Conditional Validation Skipping in CreateListingTest

    tests/Feature/GraphQL/Mutations/CreateListingTest.php

  • Added a test case to ensure proper behavior when skipping validation.
  • +45/-1   
    FillListingTest.php
    Test Skip Validation Feature in FillListingTest                   

    tests/Feature/GraphQL/Mutations/FillListingTest.php

  • Introduced a test to check mutation behavior with skipped validation.
  • +12/-0   
    FinalizeAuctionTest.php
    Validate Skip Validation Handling in FinalizeAuctionTest 

    tests/Feature/GraphQL/Mutations/FinalizeAuctionTest.php

    • Added testing for the skip validation feature in mutation.
    +12/-0   
    PlaceBidTest.php
    Test Skip Validation in PlaceBidMutation                                 

    tests/Feature/GraphQL/Mutations/PlaceBidTest.php

    • Added a test to verify the skip validation functionality.
    +16/-0   

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Description updated to latest commit (b608489)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    4, due to the complexity of the changes involving multiple files and the introduction of conditional validation logic which requires careful review to ensure correctness and security.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Possible Bug: The rulesWithoutValidation method in various mutations should ensure that the ValidHex rule is applied consistently to all relevant fields to prevent malformed inputs.

    Inconsistency: The rulesWithValidation and rulesWithoutValidation methods across different mutations might lead to inconsistent validation behaviors if not properly managed and tested, especially with the introduction of the skipValidation logic.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesrc/GraphQL/Mutations/CancelListingMutation.php
    suggestion      

    Consider adding a check in rulesWithoutValidation to ensure that listingId is always validated against a hexadecimal pattern, even when other validations are skipped. This can prevent issues with malformed IDs being processed. [important]

    relevant linenew ValidHex(32),

    relevant filesrc/GraphQL/Mutations/CreateListingMutation.php
    suggestion      

    Refactor the makeOrTakeRuleExist method to ensure it is used consistently across different parts of the code, especially in the rulesWithValidation method to maintain validation integrity. [important]

    relevant lineprotected function makeOrTakeRuleExist(?string $collectionId = null, ?bool $isMake = true): array

    relevant filesrc/GraphQL/Mutations/FillListingMutation.php
    suggestion      

    In rulesWithoutValidation, ensure that the ValidHex rule is applied to listingId to prevent processing invalid hexadecimal values, which could lead to errors or security issues. [important]

    relevant linenew ValidHex(32),

    relevant filesrc/GraphQL/Mutations/PlaceBidMutation.php
    suggestion      

    Ensure that the rulesWithoutValidation method includes necessary checks for price to prevent extremely low or high values that could affect the application's stability or lead to economic exploits. [important]

    relevant linenew MinBigInt(1),


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Rename the method to improve naming consistency and clarity.

    Consider renaming the method rulesWithValidation to getValidationRules to maintain
    consistency with other method naming conventions and to clearly indicate that the method
    returns validation rules.

    src/GraphQL/Mutations/CancelListingMutation.php [97]

    -protected function rulesWithValidation(array $args): array
    +protected function getValidationRules(array $args): array
     
    Improve handling of potential null values in auctionData to prevent runtime errors.

    Consider using a more robust approach to handle potential null values from Arr::get when
    setting auctionData. This will ensure that the AuctionDataParams constructor does not
    receive null values unexpectedly, which could lead to runtime errors.

    tests/Feature/GraphQL/Mutations/CreateListingTest.php [50-52]

    -$params['auctionData'] = (Arr::get($params, 'auctionData'))
    -    ? new AuctionDataParams(Arr::get($params, 'auctionData.startBlock'), Arr::get($params, 'auctionData.endBlock'))
    +$auctionData = Arr::get($params, 'auctionData', []);
    +$params['auctionData'] = !empty($auctionData)
    +    ? new AuctionDataParams($auctionData['startBlock'] ?? null, $auctionData['endBlock'] ?? null)
         : null;
     
    Add assertions for response status and content type to ensure API response correctness.

    Add assertions to check the response status and content type to ensure the API's
    robustness and correctness in handling requests, especially when skipValidation is used.

    tests/Feature/GraphQL/Mutations/CreateListingTest.php [64]

     $response = $this->graphql(
         $this->method,
         $params = [
             'makeAssetId' => [
                 'collectionId' => fake()->numberBetween(10000, 20000),
                 'tokenId' => ['integer' => fake()->numberBetween(10000, 20000)],
             ],
             ...
         ]
     );
    +$this->assertEquals(200, $response->getStatusCode());
    +$this->assertEquals('application/json', $response->headers->get('Content-Type'));
     
    Maintainability
    Refactor the method to use a string parameter for asset type instead of a boolean.

    Refactor the makeOrTakeRuleExist method to accept an additional parameter for specifying
    the asset type ('make' or 'take'), instead of using a boolean flag. This will improve
    readability and maintainability of the code.

    src/GraphQL/Mutations/CreateListingMutation.php [151]

    -protected function makeOrTakeRuleExist(?string $collectionId = null, ?bool $isMake = true): array
    +protected function getAssetRule(?string $collectionId = null, string $assetType = 'make'): array
     
    Combine similar methods into one with a parameter to toggle validation.

    Combine the rulesWithValidation and rulesWithoutValidation methods into a single method
    with a parameter to toggle validation. This reduces redundancy and improves code
    organization.

    src/GraphQL/Mutations/PlaceBidMutation.php [105-126]

    -protected function rulesWithValidation(array $args): array
    -protected function rulesWithoutValidation(array $args): array
    +protected function getRules(array $args, bool $withValidation = true): array
     
    Extract repeated validation rules into a reusable method or configuration.

    To avoid code duplication, extract the repeated validation rules into a separate method or
    configuration that can be reused across different methods or classes.

    src/GraphQL/Mutations/FinalizeAuctionMutation.php [115-119]

    -'listingId' => [
    -    'bail',
    -    'filled',
    -    'max:255',
    -    new ValidHex(32),
    -],
    +'listingId' => $this->commonListingIdRules(),
     
    Refactor repeated asset ID creation logic into a separate method to improve code maintainability.

    Refactor the repeated logic for encoding token IDs and creating asset ID objects into a
    separate method to reduce code duplication and improve maintainability.

    tests/Feature/GraphQL/Mutations/CreateListingTest.php [86-92]

    -$params['makeAssetId'] = new MultiTokensTokenAssetIdParams(
    -    Arr::get($params, 'makeAssetId.collectionId'),
    -    $this->encodeTokenId(Arr::get($params, 'makeAssetId'))
    -);
    -$params['takeAssetId'] = new MultiTokensTokenAssetIdParams(
    -    Arr::get($params, 'takeAssetId.collectionId'),
    -    $this->encodeTokenId(Arr::get($params, 'takeAssetId'))
    -);
    +private function createAssetIdParams($assetIdKey) {
    +    return new MultiTokensTokenAssetIdParams(
    +        Arr::get($this->params, "{$assetIdKey}.collectionId"),
    +        $this->encodeTokenId(Arr::get($this->params, $assetIdKey))
    +    );
    +}
     
    +$params['makeAssetId'] = $this->createAssetIdParams('makeAssetId');
    +$params['takeAssetId'] = $this->createAssetIdParams('takeAssetId');
    +
    Security
    Add backend checks for skipValidation to prevent unauthorized actions.

    Ensure that the skipValidation parameter is properly handled in the backend to prevent
    security risks such as unauthorized actions or data integrity issues.

    tests/Feature/GraphQL/Mutations/CreateListingTest.php [82]

    -'skipValidation' => true,
    +// Ensure backend validation logic checks:
    +if ($params['skipValidation'] && !currentUserHasPermission('skip_validation')) {
    +    throw new UnauthorizedActionException('You do not have permission to skip validation.');
    +}
     
    Best practice
    Replace direct usage of fake() with a factory method to improve data consistency in tests.

    Use a factory or a more sophisticated mocking approach for generating test data to ensure
    consistency and maintainability of test cases.

    tests/Feature/GraphQL/Mutations/CreateListingTest.php [68-73]

    -'collectionId' => fake()->numberBetween(10000, 20000),
    -'tokenId' => ['integer' => fake()->numberBetween(10000, 20000)],
    +'collectionId' => $this->assetIdFactory->createCollectionId(),
    +'tokenId' => ['integer' => $this->assetIdFactory->createTokenId()],
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @leonardocustodio leonardocustodio merged commit f48f221 into master Apr 21, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1735 branch April 21, 2024 23:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants