Skip to content

fix: prevent overwriting existing migrations when registering new ones#1141

Merged
hwbrzzl merged 4 commits into
goravel:masterfrom
ahmed3mar:hotfix/schema-register
Jul 24, 2025
Merged

fix: prevent overwriting existing migrations when registering new ones#1141
hwbrzzl merged 4 commits into
goravel:masterfrom
ahmed3mar:hotfix/schema-register

Conversation

@ahmed3mar

@ahmed3mar ahmed3mar commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

📑 Description

Closes https://github.com/goravel/goravel/issues/

When uses Register several times to overrides the registered ones!

✅ Checks

  • Added test cases for my code

@ahmed3mar
ahmed3mar marked this pull request as ready for review July 24, 2025 07:55
Copilot AI review requested due to automatic review settings July 24, 2025 07:55
@ahmed3mar
ahmed3mar requested a review from a team as a code owner July 24, 2025 07:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the schema migration registration system where calling Register multiple times would overwrite previously registered migrations instead of accumulating them. The fix changes the behavior to append new migrations to the existing list rather than replacing it entirely.

Key changes:

  • Modified the Register method to append migrations instead of overwriting them
  • Added nil check initialization for the migrations slice

Comment thread database/schema/schema.go Outdated
r.migrations = make([]contractsschema.Migration, 0)
}

r.migrations = append(r.migrations, migrations...)

Copilot AI Jul 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation may allow duplicate migrations to be registered if the same migration is passed to Register multiple times. Consider checking for duplicates before appending to prevent potential issues with migration execution.

Copilot uses AI. Check for mistakes.
Comment thread database/schema/schema.go Outdated
Comment on lines +394 to +397
if r.migrations == nil {
r.migrations = make([]contractsschema.Migration, 0)
}

Copilot AI Jul 24, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The nil check should ideally be handled during schema initialization rather than in every Register call. This adds unnecessary overhead when Register is called multiple times.

Suggested change
if r.migrations == nil {
r.migrations = make([]contractsschema.Migration, 0)
}

Copilot uses AI. Check for mistakes.
@codecov

codecov Bot commented Jul 24, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.44%. Comparing base (e1b9bf0) to head (f3ef98e).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1141   +/-   ##
=======================================
  Coverage   68.44%   68.44%           
=======================================
  Files         221      221           
  Lines       14319    14319           
=======================================
  Hits         9800     9800           
  Misses       4144     4144           
  Partials      375      375           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread database/schema/schema.go Outdated
Comment on lines +41 to +43
if migrations == nil {
migrations = make([]contractsschema.Migration, 0)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This judgment is unnecessary.

@hwbrzzl

hwbrzzl commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

Hey @ahmed3mar I left a message in Discord, please take a look.

@hwbrzzl hwbrzzl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@hwbrzzl
hwbrzzl merged commit f9670e1 into goravel:master Jul 24, 2025
14 checks passed
@ahmed3mar
ahmed3mar deleted the hotfix/schema-register branch July 24, 2025 08:52
@hwbrzzl

hwbrzzl commented Jul 24, 2025

Copy link
Copy Markdown
Contributor

Hey @ahmed3mar I sent a private message in Discord, want to confirm your country and send a v1.16 commemorative T-shirt to you. Please contact me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants