-
Notifications
You must be signed in to change notification settings - Fork 108
feat: [#546] artisan command up and down to set website Maintenance mode #1198
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
base: master
Are you sure you want to change the base?
feat: [#546] artisan command up and down to set website Maintenance mode #1198
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1198 +/- ##
==========================================
- Coverage 70.27% 70.25% -0.02%
==========================================
Files 281 284 +3
Lines 17121 17246 +125
==========================================
+ Hits 12031 12116 +85
- Misses 4579 4610 +31
- Partials 511 520 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is a fantastic PR with a very useful addition to the framework! 🎉 I have a suggestion: how about allowing an optional |
|
|
||
| func (s *DownCommandTestSuite) TestHandle() { | ||
| app := mocksfoundation.NewApplication(s.T()) | ||
| tmpfile := filepath.Join(os.TempDir(), "/down") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In unit tests, use t.TempDir() instead of os.TempDir().
t.TempDir() is preferred in unit tests because it creates a unique temporary directory for each test and automatically cleans it up after the test finishes. This helps prevent conflicts and side effects between tests, making your tests safer and more reliable.
|
Please fix the windows CI. |
|
Thanks, great job 👍 Could add some related screenshots in the PR description? |
| func CheckForMaintenance() http.Middleware { | ||
| return func(ctx http.Context) { | ||
| if file.Exists(path.Storage("framework/down")) { | ||
| ctx.Request().AbortWithStatus(http.StatusServiceUnavailable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ctx.Request().AbortWithStatus(http.StatusServiceUnavailable) | |
| ctx.Request().Abort(http.StatusServiceUnavailable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
foundation/console/down_command.go
Outdated
|
|
||
| // Handle Execute the console command. | ||
| func (r *DownCommand) Handle(ctx console.Context) error { | ||
| path := r.app.StoragePath("framework/down") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| path := r.app.StoragePath("framework/down") | |
| path := r.app.StoragePath("framework/maintenance") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
foundation/console/down_command.go
Outdated
|
|
||
| // Extend The console command extend. | ||
| func (r *DownCommand) Extend() command.Extend { | ||
| return command.Extend{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is reason still required in this case? @almas-x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I can implement those options @hwbrzzl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compared to Laravel, the current implementation of reason is indeed a bit limited. I think we can refer to Laravel's implementation and support all the options in your screenshot except for the two that are crossed out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. What should be the response would be when no --render option is provided?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning the current logic should be fine.
3479a4b to
b70c346
Compare
e635bf5 to
07bc616
Compare
a224b58 to
63f223a
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces up and down artisan commands to manage maintenance mode, along with a middleware to enforce it. The implementation is a good start, but there are several areas for improvement. My review focuses on enhancing error handling within the new commands to provide clearer feedback for scripting and automation, and on improving the test coverage for the new maintenance middleware to ensure all its features are working as expected. I've also identified a few issues in a test helper function that could lead to flaky tests.
| hash, err := r.app.MakeHash().Make(secret) | ||
| if err != nil { | ||
| ctx.Error("Unable to generate and hash the secret") | ||
| } else { | ||
| options.Secret = hash | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When hashing the provided secret fails, an error is printed, but the command continues its execution. This can lead to the application entering maintenance mode without the intended secret protection, which could be a security risk if the secret is expected for bypass. The function should return the error to halt execution.
| hash, err := r.app.MakeHash().Make(secret) | |
| if err != nil { | |
| ctx.Error("Unable to generate and hash the secret") | |
| } else { | |
| options.Secret = hash | |
| } | |
| hash, err := r.app.MakeHash().Make(secret) | |
| if err != nil { | |
| ctx.Error("Unable to generate and hash the secret") | |
| return err | |
| } | |
| options.Secret = hash |
| func TestMaintenanceMode(t *testing.T) { | ||
| server := httptest.NewServer(testHttpCheckForMaintenanceMiddleware(nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) { | ||
| }))) | ||
| defer server.Close() | ||
|
|
||
| client := &nethttp.Client{} | ||
|
|
||
| err := file.Create(path.Storage("framework/maintenance"), "") | ||
| require.NoError(t, err) | ||
|
|
||
| resp, err := client.Get(server.URL) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) | ||
|
|
||
| err = file.Remove(path.Storage("framework/maintenance")) | ||
| require.NoError(t, err) | ||
|
|
||
| resp, err = client.Get(server.URL) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test for CheckForMaintenance middleware is incomplete. It only covers the case where an empty maintenance file exists, which causes a JSON unmarshalling error and results in a 503 status. The test suite should be expanded to cover all the features of the maintenance mode:
- A valid maintenance file with a
reasonand customstatus. - The
redirectoption. - The
renderoption. - The
secretbypass mechanism.
Without these tests, the middleware's core logic is not verified.
| func (r *TestRequest) Input(key string, defualtVaue ...string) string { | ||
| if body, err := io.ReadAll(r.ctx.request.Body); err != nil { | ||
| if len(defualtVaue) > 0 { | ||
| return defualtVaue[0] | ||
| } | ||
| return "" | ||
| } else { | ||
| data := map[string]any{} | ||
| if err := json.Unmarshal(body, &data); err != nil { | ||
| return "" | ||
| } | ||
| if data[key] == nil { | ||
| return "" | ||
| } | ||
| return data[key].(string) | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation of Input has several issues:
- Request body consumption:
io.ReadAll(r.ctx.request.Body)consumes the request body. Subsequent calls toInput(or any other method that reads the body) within the same request lifecycle will find an empty body, leading to unpredictable test failures. The body should be read once and cached. - Unsafe type assertion:
data[key].(string)is an unsafe type assertion that will panic if the value is not a string (e.g., a number). You should use the two-variable form:val, ok := data[key].(string). - Error handling: If
json.Unmarshalfails, it returns an empty string, silently swallowing the error. This can make debugging tests difficult. - Content-Type assumption: The method assumes the request body is JSON. It doesn't handle other common content types like
application/x-www-form-urlencoded.
A more robust implementation should be considered to avoid these problems.
| ctx.Error("Unable to generate and hash the secret") | ||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When hashing the auto-generated secret fails, an error is printed, but the function returns nil, indicating success. This is misleading and prevents proper error handling by the caller. The function should propagate the underlying error.
| ctx.Error("Unable to generate and hash the secret") | |
| return nil | |
| ctx.Error("Unable to generate and hash the secret") | |
| return err |
| content, err := file.GetContent(tmpfile) | ||
|
|
||
| assert.Nil(s.T(), err) | ||
|
|
||
| var maintenanceOptions *MaintenanceOptions | ||
| err = json.Unmarshal([]byte(content), &maintenanceOptions) | ||
| assert.Nil(s.T(), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test and others (TestHandleWithReason, TestHandleWithRedirect, TestHandleWithRender, TestHandleSecret), file.GetContent is used to read the file, which returns a string. This string is then converted back to a byte slice for json.Unmarshal. It would be more efficient and consistent with TestHandleWithSecret to use os.ReadFile directly, which returns a byte slice.
| content, err := file.GetContent(tmpfile) | |
| assert.Nil(s.T(), err) | |
| var maintenanceOptions *MaintenanceOptions | |
| err = json.Unmarshal([]byte(content), &maintenanceOptions) | |
| assert.Nil(s.T(), err) | |
| content, err := os.ReadFile(tmpfile) | |
| assert.Nil(s.T(), err) | |
| var maintenanceOptions *MaintenanceOptions | |
| err = json.Unmarshal(content, &maintenanceOptions) | |
| assert.Nil(s.T(), err) |
|
|
||
| ctx.Error("The application is not in maintenance mode") | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the application is not in maintenance mode, the command prints an error but returns nil, indicating success. This can be misleading for scripts that rely on exit codes. It would be better to return an error to signal that the command's intended action was not performed. You will also need to import the fmt package.
| return nil | |
| return fmt.Errorf("the application is not in maintenance mode") |
| return | ||
| } | ||
|
|
||
| // Checking err to suppress the linter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is a code smell. It implies that the error check is only present to satisfy the linter, not for actual error handling. If the error is not important, it should be explicitly ignored with _ = .... If it is important, it should be handled properly (e.g., logged), and this comment should be removed.
hwbrzzl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, great PR! Just left a few questions.
|
|
||
| // Handle Execute the console command. | ||
| func (r *DownCommand) Handle(ctx console.Context) error { | ||
| path := r.app.StoragePath("framework/maintenance") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use path.Storage() instead.
| } | ||
|
|
||
| if render := ctx.Option("render"); render != "" { | ||
| if r.app.MakeView().Exists(render) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please inject View to the command directly instead of App, the same Hash.
| if r.app.MakeView().Exists(render) { | ||
| options.Render = render | ||
| } else { | ||
| ctx.Error("Unable to find the view template") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the error to errors/list.go, the same other errors.
| if redirect := ctx.Option("redirect"); redirect != "" { | ||
| options.Redirect = redirect | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if redirect := ctx.Option("redirect"); redirect != "" { | |
| options.Redirect = redirect | |
| } | |
| options.Redirect = ctx.Option("redirect") |
| if secret := ctx.Option("secret"); secret != "" { | ||
| hash, err := r.app.MakeHash().Make(secret) | ||
| if err != nil { | ||
| ctx.Error("Unable to generate and hash the secret") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil here.
| "github.com/goravel/framework/support/path" | ||
| ) | ||
|
|
||
| func CheckForMaintenance() http.Middleware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| func CheckForMaintenance() http.Middleware { | |
| func CheckForMaintenanceMode() http.Middleware { |
| content, err := os.ReadFile(filepath) | ||
|
|
||
| if err != nil { | ||
| ctx.Request().Abort(http.StatusServiceUnavailable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ctx.Request().Abort(http.StatusServiceUnavailable) | |
| ctx.Response().String(http.StatusServiceUnavailable, err.Error()).Abort() |
| } | ||
|
|
||
| if err = ctx.Response().Redirect(http.StatusTemporaryRedirect, maintenanceOptions.Redirect).Abort(); err != nil { | ||
| return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about panic in such cases? The panic will be caught and logged.
| }) | ||
| } | ||
|
|
||
| func TestMaintenanceMode(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cover more cases.
|
|
||
| func testHttpCheckForMaintenanceMiddleware(next nethttp.Handler) nethttp.Handler { | ||
| return nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) { | ||
| CheckForMaintenance()(NewTestContext(r.Context(), next, w, r)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use a mock context instead of creating a test one.

📑 Description
This PR adds two Artisan commands
The
downcommands creates a temporary file in the framework storage folderThe
upcommands removes that file if existsThere is an another middleware
CheckForMaintenance, that checks for the file.It returns the request with
503status code if the file exists else allow the request to passthrough.Closes goravel/goravel#546
✅ Checks