Skip to content

fix: [#672] facades.Orm() doesn't refresh after calling facades.Testing().Docker().Database()#1033

Merged
hwbrzzl merged 3 commits intov1.15.xfrom
bowen/#672
May 15, 2025
Merged

fix: [#672] facades.Orm() doesn't refresh after calling facades.Testing().Docker().Database()#1033
hwbrzzl merged 3 commits intov1.15.xfrom
bowen/#672

Conversation

@hwbrzzl
Copy link
Contributor

@hwbrzzl hwbrzzl commented May 14, 2025

📑 Description

Closes goravel/goravel#672

This pull request introduces several changes across multiple files to improve code consistency, enhance functionality, and simplify maintenance. The most significant updates include centralizing binding constants, refactoring service provider methods for clarity, adding new functionalities to caching and database interfaces, and enhancing test cases for better maintainability.

Refactoring and Centralization of Binding Constants

  • Introduced a new contracts/binding.go file to centralize all binding constants, replacing hardcoded string bindings across the codebase.
  • Updated service providers (auth, cache, config, console, crypt) to use the centralized binding constants for cleaner and more maintainable code. [1] [2] [3] [4] [5]

Enhancements to Caching and Locking

  • Added a new BlockWithTicker method to the Lock interface and its implementation, allowing for customizable retry intervals when attempting to acquire a lock. [1] [2]
  • Updated the Driver interface documentation to clarify the behavior of the Lock method when expiration is not set.

Database Schema and Query Improvements

  • Added support for creating boolean and custom type columns in the Blueprint interface.
  • Enhanced the Grammar interface to include support for boolean column definitions.
  • Replaced the QueryWithSetContext interface with QueryWithContext for better naming consistency and functionality.

Updates to Console Application

  • Refactored the NewApplication function in console/application.go to accept a useArtisan flag, improving flexibility in command handling.
  • Updated related test cases to reflect the new useArtisan parameter. [1] [2]

New Features and Interfaces

  • Added a new Store interface in contracts/http/store.go to support rate-limiting mechanisms.

These changes collectively improve code readability, maintainability, and functionality while introducing new capabilities to the framework.

✅ Checks

  • Added test cases for my code

@hwbrzzl hwbrzzl marked this pull request as ready for review May 14, 2025 13:47
Copilot AI review requested due to automatic review settings May 14, 2025 13:47
@hwbrzzl hwbrzzl requested a review from a team as a code owner May 14, 2025 13:47
Copy link
Contributor

Copilot AI left a comment

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 addresses issue #672 by ensuring the ORM facade is properly refreshed after nested facade calls, and by standardizing the refresh mechanism across the container and application.

  • Renamed the container’s Refresh method to Fresh (and updated tests/mocks accordingly).
  • Added an Application.Refresh() helper that invokes the container refresh and reboots service providers.
  • Refactored ORM initialization to use the new no-arg refresh signature and adjusted service provider error handling.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
mocks/foundation/Application.go Updated mock Refresh to a no-arg signature.
foundation/container.go Renamed Refresh to Fresh and updated implementation.
foundation/container_test.go Renamed tests from Refresh to Fresh.
foundation/application.go Added Application.Refresh() and repositioned setTimezone.
database/service_provider.go Changed error branch to return a fallback ORM instance.
database/orm/orm.go Updated Refresh signature and BuildOrm fallback logic.
contracts/foundation/application.go Updated Refresh signature and simplified its doc comment.
Comments suppressed due to low confidence (1)

foundation/application.go:95

  • There are no existing tests for Application.Refresh(). Consider adding a unit test to confirm it invokes both the container refresh and service provider bootstrapping as expected.
func (app *Application) Refresh() {

@codecov
Copy link

codecov bot commented May 14, 2025

Codecov Report

Attention: Patch coverage is 11.11111% with 8 lines in your changes missing coverage. Please review.

Please upload report for BASE (v1.15.x@a8aa8fe). Learn more about missing BASE report.

Files with missing lines Patch % Lines
foundation/application.go 0.00% 4 Missing ⚠️
database/orm/orm.go 0.00% 3 Missing ⚠️
database/service_provider.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             v1.15.x    #1033   +/-   ##
==========================================
  Coverage           ?   68.67%           
==========================================
  Files              ?      218           
  Lines              ?    18803           
  Branches           ?        0           
==========================================
  Hits               ?    12913           
  Misses             ?     5228           
  Partials           ?      662           

☔ 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.

Copy link
Contributor

@almas-x almas-x left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

// facades should be refresh simulaneously.
Refresh(bindings ...any)
// Refresh all modules after changing config, will call the Boot method simultaneously.
Refresh()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is not documented, it is only used internally, User shouldn't use it.

@hwbrzzl hwbrzzl merged commit f2233d7 into v1.15.x May 15, 2025
11 checks passed
@hwbrzzl hwbrzzl deleted the bowen/#672 branch May 15, 2025 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants