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-1882] Wrap codes with db transaction #55

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Jul 9, 2024

PR Type

Bug fix


Description

  • Wrapped the DispatchMutation resolve method with explicit database transaction handling using DB::beginTransaction() and DB::commit().
  • Simplified the return statement by removing the nested transaction call and storing the transaction result in a variable.

Changes walkthrough 📝

Relevant files
Bug fix
DispatchMutation.php
Wrap DispatchMutation resolve method with database transaction

src/GraphQL/Mutations/DispatchMutation.php

  • Wrapped code execution within a database transaction.
  • Replaced inline transaction handling with explicit
    DB::beginTransaction() and DB::commit().
  • Simplified the return statement by removing the nested transaction
    call.
  • +4/-4     

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

    @enjinabner enjinabner added the bug Something isn't working label Jul 9, 2024
    @enjinabner enjinabner self-assigned this Jul 9, 2024
    Copy link

    github-actions bot commented Jul 9, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    Ensure that DB::rollback() is implemented for handling exceptions within the transaction. Without this, failed transactions may not revert changes, leading to potential data inconsistencies.

    Copy link

    github-actions bot commented Jul 9, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for the database transaction

    Add a DB::rollBack() in a catch block to handle exceptions that might occur during the
    transaction. This ensures that the database state is reverted if an error occurs after the
    transaction has started but before it is committed.

    src/GraphQL/Mutations/DispatchMutation.php [101-104]

     DB::beginTransaction();
    -$encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    -$transaction = $this->storeTransaction($args, $encodedCall);
    -DB::commit();
    +try {
    +    $encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    +    $transaction = $this->storeTransaction($args, $encodedCall);
    +    DB::commit();
    +} catch (\Exception $e) {
    +    DB::rollBack();
    +    throw $e;
    +}
     
    Suggestion importance[1-10]: 10

    Why: This suggestion is crucial for ensuring database integrity by rolling back the transaction in case of an exception, preventing partial commits and potential data corruption.

    10
    Best practice
    Ensure safe commit of transactions

    Ensure that DB::commit() is only called if there are no exceptions by placing it inside a
    try block. This prevents committing a transaction that might have encountered issues
    during execution.

    src/GraphQL/Mutations/DispatchMutation.php [102-104]

    -$encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    -$transaction = $this->storeTransaction($args, $encodedCall);
    -DB::commit();
    +try {
    +    $encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    +    $transaction = $this->storeTransaction($args, $encodedCall);
    +    DB::commit();
    +} catch (\Exception $e) {
    +    DB::rollBack();
    +    throw $e;
    +}
     
    Suggestion importance[1-10]: 9

    Why: This suggestion is important for maintaining data integrity by ensuring that the commit only occurs if no exceptions are thrown, which aligns with best practices for transaction management.

    9
    Enhancement
    Verify transaction storage success before committing

    Consider checking the success of storeTransaction before committing the transaction. This
    can prevent committing incomplete or incorrect data.

    src/GraphQL/Mutations/DispatchMutation.php [103-104]

     $transaction = $this->storeTransaction($args, $encodedCall);
    -DB::commit();
    +if ($transaction) {
    +    DB::commit();
    +} else {
    +    DB::rollBack();
    +    throw new \Exception("Failed to store transaction");
    +}
     
    Suggestion importance[1-10]: 8

    Why: This enhancement adds an additional layer of validation to ensure that the transaction is only committed if the storeTransaction method succeeds, which helps in maintaining data accuracy.

    8
    Maintainability
    Implement logging for transaction processes

    Add logging for the transaction process to aid in debugging and monitoring the transaction
    states.

    src/GraphQL/Mutations/DispatchMutation.php [101-104]

     DB::beginTransaction();
    -$encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    -$transaction = $this->storeTransaction($args, $encodedCall);
    -DB::commit();
    +Log::info("Transaction started");
    +try {
    +    $encodedCall = static::getFuelTankCall($this->getMethodName(), $args);
    +    $transaction = $this->storeTransaction($args, $encodedCall);
    +    DB::commit();
    +    Log::info("Transaction committed");
    +} catch (\Exception $e) {
    +    DB::rollBack();
    +    Log::error("Transaction failed: " . $e->getMessage());
    +    throw $e;
    +}
     
    Suggestion importance[1-10]: 7

    Why: Adding logging improves maintainability by providing insights into the transaction process, which can be valuable for debugging and monitoring purposes.

    7

    @leonardocustodio
    Copy link
    Member

    Should we use simulate = true in the first call? That way it doesn't save

    @enjinabner
    Copy link
    Contributor Author

    Should we use simulate = true in the first call? That way it doesn't save

    Simulate will bypass the validations, i think we need all of that.

    @leonardocustodio
    Copy link
    Member

    Are you sure? I thought only skipValidation bypassed the validations

    @enjinabner
    Copy link
    Contributor Author

    Are you sure? I thought only skipValidation bypassed the validations

    Right, i totally forgot about that. But still, i think wrapping with transaction is still the safest. If there's something being committed in the DB while getting the encoded call and the mutation crashed, it can be rolled back.

    Cases like this, is what i meant, but this one should be okay.
    Screenshot 2024-07-11 at 9 24 29 AM

    @leonardocustodio leonardocustodio merged commit d75a591 into master Jul 11, 2024
    7 checks passed
    @leonardocustodio leonardocustodio deleted the feature/pla-1882/fix-dispatch-and-touch branch July 11, 2024 12:33
    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.

    2 participants