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.

Sign up for GitHub

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

Jump to bottom

[PLA-1733] Adds skipValidation to all transaction mutations #41

Merged
merged 4 commits into from
Apr 18, 2024
Merged

[PLA-1733] Adds skipValidation to all transaction mutations #41

merged 4 commits into from
Apr 18, 2024

Conversation

leonardocustodio
Copy link
Member

@leonardocustodio leonardocustodio commented Apr 18, 2024 •
edited by github-actions bot

Type

enhancement

Description

  • Added HasSkippableRules trait to various mutations to support skipping validation rules.
  • Introduced getSkipValidationField method in mutations to handle skippable validation rules.
  • Refactored existing validation rules into rulesWithValidation and added new rulesWithoutValidation for cases without DB rules across multiple mutations.
  • Modified HasFuelTankValidationRules trait to include new methods for handling validation rules with and without database checks.

Changes walkthrough

Relevant files
Enhancement
14 files
AddAccountMutation.php
Integrate Skippable Validation Rules in AddAccountMutation

src/GraphQL/Mutations/AddAccountMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +26/-2   
    BatchAddAccountMutation.php
    Integrate Skippable Validation Rules in BatchAddAccountMutation

    src/GraphQL/Mutations/BatchAddAccountMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +22/-2   
    BatchRemoveAccountMutation.php
    Integrate Skippable Validation Rules in BatchRemoveAccountMutation

    src/GraphQL/Mutations/BatchRemoveAccountMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +27/-2   
    CreateFuelTankMutation.php
    Integrate Skippable Validation Rules in CreateFuelTankMutation

    src/GraphQL/Mutations/CreateFuelTankMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
    		•  +3/-0     
    DestroyFuelTankMutation.php
    Integrate Skippable Validation Rules in DestroyFuelTankMutation

    src/GraphQL/Mutations/DestroyFuelTankMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +20/-2   
    DispatchMutation.php
    Integrate Skippable Validation Rules in DispatchMutation 

    src/GraphQL/Mutations/DispatchMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +29/-2   
    ForceSetConsumptionMutation.php
    Integrate Skippable Validation Rules in ForceSetConsumptionMutation

    src/GraphQL/Mutations/ForceSetConsumptionMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +42/-2   
    InsertRuleSetMutation.php
    Integrate Skippable Validation Rules in InsertRuleSetMutation

    src/GraphQL/Mutations/InsertRuleSetMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +26/-2   
    MutateFuelTankMutation.php
    Integrate Skippable Validation Rules in MutateFuelTankMutation

    src/GraphQL/Mutations/MutateFuelTankMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +20/-2   
    RemoveAccountMutation.php
    Integrate Skippable Validation Rules in RemoveAccountMutation

    src/GraphQL/Mutations/RemoveAccountMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +26/-2   
    RemoveAccountRuleDataMutation.php
    Integrate Skippable Validation Rules in RemoveAccountRuleDataMutation

    src/GraphQL/Mutations/RemoveAccountRuleDataMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +30/-2   
    RemoveRuleSetMutation.php
    Integrate Skippable Validation Rules in RemoveRuleSetMutation

    src/GraphQL/Mutations/RemoveRuleSetMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +25/-2   
    ScheduleMutateFreezeStateMutation.php
    Integrate Skippable Validation Rules in
    ScheduleMutateFreezeStateMutation

    src/GraphQL/Mutations/ScheduleMutateFreezeStateMutation.php

  • Added trait HasSkippableRules to support skipping validation rules.
  • Introduced getSkipValidationField in the arguments method.
  • Refactored validation rules into rulesWithValidation and added
    rulesWithoutValidation for cases without DB rules.
    		•  +26/-2   
    HasFuelTankValidationRules.php
    Refactor and Add Validation Rules Methods in
    HasFuelTankValidationRules Trait

    src/GraphQL/Traits/HasFuelTankValidationRules.php

  • Modified method commonRules to commonRulesExist and added a new
    commonRules method for common validation rules.
  • Added dispatchRulesExist method for dispatch rules validation with DB
    rules.
    		•  +75/-4   

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

    @leonardocustodio leonardocustodio added the enhancement New feature or request label Apr 18, 2024
    @leonardocustodio leonardocustodio self-assigned this Apr 18, 2024
    @github-actions GitHub Actions
    Copy link

    PR Description updated to latest commit (64ea758)

    @github-actions GitHub Actions
    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple files and significant changes related to validation logic and trait integration across various mutations. Understanding the context and ensuring the new validation rules are correctly implemented requires a moderate level of effort.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The method rulesWithoutValidation in various mutations might not handle edge cases where args are null or not passed, which could lead to PHP errors. Example: protected function rulesWithoutValidation(array $args): array

    🔒 Security concerns

    No

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

    Consider adding null checks or default values for $args in rulesWithoutValidation to prevent potential errors when $args is not provided. [important]
    relevant lineprotected function rulesWithoutValidation(array $args): array
    relevant filesrc/GraphQL/Mutations/BatchAddAccountMutation.php
    suggestion      

    Ensure that the rulesWithValidation method correctly merges additional validation rules when extending or overriding to prevent any missing validation scenarios. [important]
    relevant lineprotected function rulesWithValidation(array $args): array
    relevant filesrc/GraphQL/Mutations/DestroyFuelTankMutation.php
    suggestion      

    Add exception handling for the new validation rules methods to manage unexpected inputs or database errors gracefully. [important]
    relevant lineprotected function rulesWithoutValidation(array $args): array
    relevant filesrc/GraphQL/Mutations/DispatchMutation.php
    suggestion      

    Optimize the rulesWithoutValidation by caching the results of expensive operations like new ValidSubstrateAddress() if used frequently across requests. [medium]
    relevant lineprotected function rulesWithoutValidation(array $args): array
    ✨ 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.

    @github-actions GitHub Actions
    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Add validation for 'userId' in the rulesWithValidation method.

    Consider adding a validation rule for 'userId' in the rulesWithValidation method to ensure
    it is not empty and meets the required constraints.

    src/GraphQL/Mutations/AddAccountMutation.php [115-116]

     'tankId' => [
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
 ],
+'userId' => [
+    'bail',
+    'filled',
+    'max:255',
+    new ValidSubstrateAddress(),
+],
    Strengthen 'tankId' validation in the rulesWithValidation method.

    Add explicit validation for 'tankId' to ensure it is not only filled and has a maximum
    length but also checks for specific conditions or formats if needed.

    src/GraphQL/Mutations/DestroyFuelTankMutation.php [106-107]

     'tankId' => [
+    'required',
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
+    // Additional custom validation rules if necessary
 ],
    Add numeric and non-negative validation for 'totalConsumed'.

    Consider adding a validation for 'totalConsumed' to ensure it adheres to expected
    numerical ranges and is not negative.

    src/GraphQL/Mutations/ForceSetConsumptionMutation.php [205-206]

     'totalConsumed' => [
     'bail',
+    'numeric',
+    'min:0',
     new MinBigInt(),
     new MaxBigInt(),
 ],
    Add validation for the ruleSetId field to ensure data integrity.

    Consider adding validation for the ruleSetId field in the rulesWithValidation method to
    ensure data integrity and prevent potential issues.

    src/GraphQL/Mutations/InsertRuleSetMutation.php [138]

     'tankId' => [
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
 ],
+'ruleSetId' => [
+    'required',
+    new MinBigInt(),
+    new MaxBigInt(Hex::MAX_UINT32),
+],
    Ensure ruleSetId is mandatory by adding a 'required' rule.

    For better data validation, add a 'required' rule for ruleSetId in the rulesWithValidation
    method to ensure it is always included in requests.

    src/GraphQL/Mutations/RemoveRuleSetMutation.php [138]

     'tankId' => [
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
 ],
+'ruleSetId' => [
+    'required',
+    new MinBigInt(),
+    new MaxBigInt(Hex::MAX_UINT32),
+],
    Security
    Review and ensure all necessary validations are included in rulesWithoutValidation.

    Ensure that the rulesWithoutValidation method does not inadvertently skip important
    validations that could lead to security or data integrity issues.

    src/GraphQL/Mutations/AddAccountMutation.php [139-140]

     'tankId' => [
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
 ],
+// Additional necessary validations here
    Add validation for the userId field to enhance security and data integrity.

    Ensure that the userId field is validated in the rulesWithValidation method to prevent
    unauthorized or malformed data submissions.

    src/GraphQL/Mutations/RemoveAccountMutation.php [134]

     'tankId' => [
     'bail',
     'filled',
     'max:255',
     new ValidSubstrateAddress(),
 ],
+'userId' => [
+    'required',
+    'max:255',
+    new ValidSubstrateAddress(),
+],
    Bug
    Ensure 'userIds' array is not empty in validation rules.

    Add a check to ensure the 'userIds' array is not empty in the rulesWithValidation method
    to prevent processing empty batches.

    src/GraphQL/Mutations/BatchAddAccountMutation.php [124]

    +'userIds' => ['required', 'array', 'min:1'],
 'userIds.*' => ['bail', 'filled', 'distinct', 'max:255', new ValidSubstrateAddress()],
    Best practice
    Ensure ruleSetId is always provided by adding a 'required' validation rule.

    It's recommended to add a 'required' validation rule for ruleSetId in the
    rulesWithoutValidation method to ensure that this field is always provided.

    src/GraphQL/Mutations/RemoveAccountRuleDataMutation.php [174]

     'ruleSetId' => [
+    'required',
     new MinBigInt(),
     new MaxBigInt(Hex::MAX_UINT32),
 ],
    Use constants for repeated values to improve code maintainability.

    Use constants for repeated literal values such as '255' and 'bail' to avoid magic numbers
    and strings, improving maintainability and reducing the risk of typos.

    src/GraphQL/Traits/HasFuelTankValidationRules.php [132-150]

     "{$attributePrefix}dispatchRules{$array}.whitelistedCollections.*" => [
-    'bail',
+    self::BAIL_RULE,
     'distinct',
-    'max:255',
+    'max:' . self::MAX_LENGTH,
     Rule::exists('collections', 'collection_chain_id'),
 ],
-"{$attributePrefix}dispatchRules{$array}.whitelistedPallets.*" => ['bail', 'distinct', 'max:255', 'filled', new ValidHex()],
+"{$attributePrefix}dispatchRules{$array}.whitelistedPallets.*" => [self::BAIL_RULE, 'distinct', 'max:' . self::MAX_LENGTH, 'filled', new ValidHex()],
    Use dependency injection for creating instances to improve testability and reduce tight coupling.

    Consider using dependency injection for the MinBigInt and MaxBigInt instances to
    facilitate testing and reduce coupling.

    src/GraphQL/Traits/HasFuelTankValidationRules.php [73-81]

     "{$attribute}.amount" => [
     'bail',
-    new MinBigInt(),
-    new MaxBigInt(Hex::MAX_UINT128),
+    $this->minBigInt,
+    $this->maxBigIntUint128,
 ],
 "{$attribute}.resetPeriod" => [
     'bail',
-    new MinBigInt(),
-    new MaxBigInt(Hex::MAX_UINT32),
+    $this->minBigInt,
+    $this->maxBigIntUint32,
 ],
    Maintainability
    Add a 'required' validation rule for ruleSetId to ensure it is always provided.

    To prevent potential data integrity issues, consider adding a 'required' validation rule
    for the ruleSetId field in the rulesWithoutValidation method.

    src/GraphQL/Mutations/ScheduleMutateFreezeStateMutation.php [160]

     'ruleSetId' => [
-    'bail',
-    'nullable',
+    'required',
     new MinBigInt(),
     new MaxBigInt(Hex::MAX_UINT32),
 ],
    Improve method naming for clarity and accuracy.

    Consider using a more descriptive method name than commonRulesExist to clarify the purpose
    of the method. The name commonRulesExist might imply a boolean return type (checking
    existence), whereas the method actually returns an array of rules.

    src/GraphQL/Traits/HasFuelTankValidationRules.php [22]

    -protected function commonRulesExist(string $attribute, array $args = []): array
+protected function getCommonRules(string $attribute, array $args = []): array
    Refactor complex method to improve readability and maintainability.

    Refactor the commonRules method to reduce complexity by extracting parts of the
    switch-case logic into separate methods. This will improve readability and
    maintainability.

    src/GraphQL/Traits/HasFuelTankValidationRules.php [71-101]

     return match (true) {
-    str_contains($attribute, 'FuelBudget') => [
-        "{$attribute}.amount" => [
-            'bail',
-            new MinBigInt(),
-            new MaxBigInt(Hex::MAX_UINT128),
-        ],
-        "{$attribute}.resetPeriod" => [
-            'bail',
-            new MinBigInt(),
-            new MaxBigInt(Hex::MAX_UINT32),
-        ],
-    ],
-    default => [
-        "{$attribute}.whitelistedCallers.*" => ['bail', 'distinct', 'max:255', 'filled', new ValidSubstrateAddress()],
-        "{$attribute}.whitelistedCallers" => ['nullable', 'array', 'min:1'],
-        "{$attribute}.requireToken.collectionId" => $isArray
-            ? Rule::forEach(function ($value, $key) {
-                return [
-                    'bail',
-                    'required_with:' . str_replace('collectionId', 'tokenId', $key),
-                    new MinBigInt(),
-                    new MaxBigInt(Hex::MAX_UINT128),
-                ];
-            })
-            : [
-                "required_with:{$attribute}.requireToken.tokenId",
-            ],
-        ...$this->getOptionalTokenFieldRules("{$attribute}.requireToken"),
-    ]
+    str_contains($attribute, 'FuelBudget') => $this->handleFuelBudgetRules($attribute),
+    default => $this->handleDefaultRules($attribute, $isArray)
 };
    Improve code readability by refactoring complex ternary operations into a method.

    Replace the ternary operation with a more readable conditional structure to improve code
    clarity, especially when dealing with complex conditions and operations.

    src/GraphQL/Traits/HasFuelTankValidationRules.php [87-98]

    -"{$attribute}.requireToken.collectionId" => $isArray
-    ? Rule::forEach(function ($value, $key) {
-        return [
-            'bail',
-            'required_with:' . str_replace('collectionId', 'tokenId', $key),
-            new MinBigInt(),
-            new MaxBigInt(Hex::MAX_UINT128),
-        ];
-    })
-    : [
-        "required_with:{$attribute}.requireToken.tokenId",
-    ],
+"{$attribute}.requireToken.collectionId" => $this->handleTokenCollectionIdRules($isArray, $attribute),
    ✨ 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 4624d5c into master Apr 18, 2024
    5 checks passed
    @leonardocustodio leonardocustodio deleted the PLA-1733 branch April 18, 2024 21:52
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @v16Studios v16Studios v16Studios approved these changes

    @enjinabner enjinabner Awaiting requested review from enjinabner

    Assignees

    @leonardocustodio leonardocustodio

    Labels
    enhancement New feature or request Review effort [1-5]: 3
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    None yet
    2 participants
    @leonardocustodio @v16Studios