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-2009] Add index to syncables #267

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Oct 14, 2024

PR Type

enhancement


Description

  • Added a new migration to create an index on the syncables table to improve query performance.
  • Registered the new migration in the CoreServiceProvider to ensure it is executed.

Changes walkthrough 📝

Relevant files
Enhancement
2024_10_14_041141_add_index_to_syncables_table.php
Add migration for indexing `syncables` table                         

database/migrations/2024_10_14_041141_add_index_to_syncables_table.php

  • Added a new migration file to create an index on the syncables table.
  • Implemented the up method to add an index on syncable_id and
    syncable_type.
  • Implemented the down method to drop the created index.
  • +27/-0   
    Configuration changes
    CoreServiceProvider.php
    Register new migration in CoreServiceProvider                       

    src/CoreServiceProvider.php

    • Registered the new migration add_index_to_syncables_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: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Index Naming
    Consider specifying a custom name for the index being created. This can help with readability and future maintenance, especially in large databases with many indexes.

    Down Migration
    Ensure that the index name used in the down method matches the actual index name created by Laravel if not explicitly specified in the up method.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Explicitly define the index name when creating it to ensure consistency and prevent errors during rollback

    Ensure that the index name used in the down method matches the default naming
    convention or explicitly specify the index name in the up method.

    database/migrations/2024_10_14_041141_add_index_to_syncables_table.php [14-24]

    -$table->index(['syncable_id', 'syncable_type']);
    +$table->index(['syncable_id', 'syncable_type'], 'syncables_syncable_id_syncable_type_index');
     ...
     $table->dropIndex('syncables_syncable_id_syncable_type_index');
    Suggestion importance[1-10]: 9

    Why: The suggestion improves maintainability by explicitly defining the index name, ensuring consistency between the up and down methods. This prevents potential errors during rollback if the default naming convention changes.

    9
    Best practice
    Add a conditional check to ensure the index does not already exist before adding it

    Consider checking if the index already exists before attempting to add it to prevent
    potential migration errors.

    database/migrations/2024_10_14_041141_add_index_to_syncables_table.php [13-14]

     Schema::table('syncables', function (Blueprint $table) {
    -    $table->index(['syncable_id', 'syncable_type']);
    +    if (!Schema::hasIndex('syncables', 'syncables_syncable_id_syncable_type_index')) {
    +        $table->index(['syncable_id', 'syncable_type']);
    +    }
     });
    Suggestion importance[1-10]: 8

    Why: This suggestion addresses a potential issue where attempting to add an index that already exists could cause migration errors. By adding a conditional check, it enhances the robustness of the migration script.

    8

    @enjinabner enjinabner merged commit 70afbf4 into master Oct 14, 2024
    6 of 7 checks passed
    @enjinabner enjinabner deleted the feature/pla-2009/add-index-syncables branch October 14, 2024 12:02
    enjinabner added a commit that referenced this pull request Oct 14, 2024
    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