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-2045] Improve query performance #273

Merged
merged 7 commits into from
Oct 21, 2024

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Oct 17, 2024

PR Type

enhancement, bug_fix


Description

  • Added a new managed attribute to the Transaction model and database schema to improve query performance.
  • Created a migration to add the managed column to the transactions table, including setting default values and updating existing records.
  • Simplified transaction filtering logic in GraphQL mutations by utilizing the new managed column.
  • Enhanced the Transaction model to automatically set the managed attribute during creation and updates.
  • Introduced a new method in the Account support class to determine if a public key is managed.

Changes walkthrough 📝

Relevant files
Enhancement
TransactionFactory.php
Add `managed` attribute to transaction definition               

database/factories/TransactionFactory.php

  • Added a new managed attribute to the transaction definition.
+1/-0     
2024_10_17_061240_add_managed_to_transactions_table.php
Add `managed` column to transactions table with migration

database/migrations/2024_10_17_061240_add_managed_to_transactions_table.php

  • Created a migration to add managed column to transactions table.
  • Set default value and index for managed column.
  • Updated existing records based on wallet_public_key.
  • +35/-0   
    MarkAndListPendingTransactionsMutation.php
    Simplify transaction filtering using `managed` column       

    src/GraphQL/Schemas/Primary/Mutations/MarkAndListPendingTransactionsMutation.php

  • Simplified query logic for filtering transactions by
    wallet_public_key.
  • Replaced complex logic with managed column check.
  • +5/-14   
    Transaction.php
    Set `managed` attribute in Transaction model lifecycle     

    src/Models/Laravel/Transaction.php

  • Added boot method to set managed attribute on creating and updating.
  • +12/-0   
    Account.php
    Add method to check managed public keys                                   

    src/Support/Account.php

    • Added isManaged method to check if a public key is managed.
    +5/-0     
    Configuration changes
    CoreServiceProvider.php
    Register migration for `managed` column in transactions   

    src/CoreServiceProvider.php

  • Registered new migration for adding managed to transactions table.
  • +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    SQL Injection:
    The PR introduces a potential SQL injection vulnerability in the migration file 'database/migrations/2024_10_17_061240_add_managed_to_transactions_table.php' where raw SQL is executed with potentially unescaped input.

    ⚡ Recommended focus areas for review

    SQL Injection Risk
    The migration script uses raw SQL queries with potential unescaped inputs, which could lead to SQL injection vulnerabilities.

    Logic Change
    The mutation logic has been significantly altered to use the 'managed' flag. This change should be carefully reviewed to ensure it aligns with business requirements and does not introduce any regressions.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Replace raw SQL with Eloquent query builder to prevent SQL injection

    Avoid using direct SQL statements for updating records due to potential SQL
    injection risks. Instead, use Eloquent's query builder methods to ensure safe
    queries.

    database/migrations/2024_10_17_061240_add_managed_to_transactions_table.php [18-22]

    -DB::statement("
    -    UPDATE transactions SET managed = 1
    -    WHERE wallet_public_key IS  NULL
    -    or wallet_public_key IN('".implode("','", Account::managedPublicKeys())."')
    -");
    +Transaction::whereNull('wallet_public_key')
    +    ->orWhereIn('wallet_public_key', Account::managedPublicKeys())
    +    ->update(['managed' => true]);
    Suggestion importance[1-10]: 9

    Why: The suggestion to replace raw SQL with Eloquent query builder methods significantly enhances security by mitigating SQL injection risks. It also improves code readability and maintainability by using Laravel's built-in features.

    9
    Enhancement
    Add input validation to the isManaged method to enhance reliability and prevent potential errors

    Add a check to ensure that the input to isManaged is not null or empty to prevent
    errors and ensure reliable functionality.

    src/Support/Account.php [93-95]

    -public static function isManaged(string $publicKey): bool
    +public static function isManaged(?string $publicKey): bool
    +{
    +    if (empty($publicKey)) {
    +        return false;
    +    }
    +    return in_array($publicKey, static::managedPublicKeys());
    +}
    Suggestion importance[1-10]: 8

    Why: Adding a check for null or empty input in the isManaged method enhances the method's robustness and prevents potential errors. This change improves the reliability of the method when handling unexpected input.

    8
    Maintainability
    Consolidate repeated logic into a single method to enhance code cleanliness and maintainability

    Refactor the repeated logic in the 'creating' and 'updating' model events into a
    single private method to improve code maintainability.

    src/Models/Laravel/Transaction.php [70-75]

    -static::creating(
    -    fn ($model) => $model->managed = empty($model->wallet_public_key) || Account::isManaged($model->wallet_public_key),
    -);
    -static::updating(
    -    fn ($model) => $model->managed = empty($model->wallet_public_key) || Account::isManaged($model->wallet_public_key),
    -);
    +static::creating([self::class, 'handleManagedAttribute']);
    +static::updating([self::class, 'handleManagedAttribute']);
     
    +private static function handleManagedAttribute($model) {
    +    $model->managed = empty($model->wallet_public_key) || Account::isManaged($model->wallet_public_key);
    +}
    +
    Suggestion importance[1-10]: 7

    Why: Refactoring the repeated logic into a single method improves code maintainability and readability by reducing duplication. This change makes future updates easier and reduces the risk of errors.

    7

    @enjinabner enjinabner marked this pull request as ready for review October 21, 2024 03:12
    @enjinabner enjinabner merged commit d64a1f5 into master Oct 21, 2024
    7 checks passed
    @enjinabner enjinabner deleted the feature/pla-2045/improve-query-performance branch October 21, 2024 10:43
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants