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.

Sign up for GitHub

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

Jump to bottom

In Auto setup, when IsUninitialized, response http status code 503 #16834

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

In Auto setup, when IsUninitialized, response http status code 503 #16834

wants to merge 34 commits into from
+317 −132

Conversation

infofromca
Copy link
Contributor

Fixes #16833

hishamco
hishamco requested changes Oct 4, 2024
View reviewed changes
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/IAutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
@hishamco hishamco requested a review from gvkries October 4, 2024 16:16
infofromca and others added 7 commits October 4, 2024 12:17
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/IAutoSe…
1573e53 
…tupService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSet…
ff715ae 
…upService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSet…
5ffc4aa 
…upService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSet…
c02123c 
…upService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSet…
281daa4 
…upService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca @hishamco
Update src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSet…
511dc12 
…upService.cs

Co-authored-by: Hisham Bin Ateya <[email protected]>
@infofromca
Move the docs to the interface
7a51f44
@hishamco
Copy link
Member

hishamco commented Oct 4, 2024

A unit of functional test will be a plus

@hishamco hishamco requested a review from Piedone October 5, 2024 22:37
@Piedone
Copy link
Member

Piedone commented Oct 6, 2024

How do you test this?

sebastienros
sebastienros reviewed Oct 10, 2024
View reviewed changes
src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs
}
}
}

httpContext.Response.Redirect(pathBase);

return;
}
else
Copy link
Member

Choose a reason for hiding this comment

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

This would prevent the error message from being displayed to the user. Like server-side validation or database creation error.

Even if you still decide to keep 503 as the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastienros : Just committed
Won't stop other Middlewares after success

@gvkries gvkries changed the title In Auto setup, when IsUninitialized, response http status code 409 In Auto setup, when IsUninitialized, response http status code 503 Oct 10, 2024
@Piedone
Copy link
Member

Piedone commented Oct 11, 2024

So @infofromca, how do you test this?

@Piedone
Copy link
Member

Piedone commented Oct 14, 2024

@infofromca When you're done addressing all feedback of a review, click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

Also, I'm still waiting for your reply on how one can test this.

@infofromca
Copy link
Contributor Author

@Piedone for your reply on how one can test this, please see the unit test

@Piedone
Copy link
Member

Piedone commented Oct 14, 2024

I'm talking about trying it out manually. How can I try this out and make sure that it works? How did you do this during development?

@infofromca
Copy link
Contributor Author

I think a new read.me will be required. Let me find the time for it, will have vacation in near future

@infofromca
Copy link
Contributor Author

infofromca commented Oct 29, 2024 •
edited
Loading

@Piedone : I just tested it manually. there was a DI error which I just corrected.
After I uncommented "OrchardCore_AutoSetup" on appsetting.json, it run well and I could login in to the admin panel.--good.
then , I made the code to return (setupContext, false); inside SetupTenantAsync of AutoSetupService, it gave 503 and the message as The AutoSetup failed installing the site 'AutoSetup Example' with errors: . For errors: . , because I did not put the errors string inside SetupTenantAsync actually. --- It seems as we expected.
屏幕截图 2024-10-29 113043

Piedone
Piedone approved these changes Oct 29, 2024
View reviewed changes
Copy link
Member

@Piedone Piedone left a comment

Choose a reason for hiding this comment

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

@hishamco @sebastienros?

src/OrchardCore.Modules/OrchardCore.AutoSetup/Services/AutoSetupService.cs Outdated Show resolved Hide resolved
sebastienros
sebastienros reviewed Oct 29, 2024
View reviewed changes
src/OrchardCore.Modules/OrchardCore.AutoSetup/AutoSetupMiddleware.cs Outdated

await httpContext.Response.WriteAsync($"The AutoSetup failed installing the site '{_setupOptions.SiteName}' with errors: {stringBuilder}.");
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't write error messages to the response directly. For instance here the SiteName is user input and could contain XSS. I believe it was only logged before. If we want to keep doing it then we don't need the tuple as a return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sebastienros : Committed new one of shouldn't write error messages to the response directly

This was referenced Nov 1, 2024
Open
Open
hishamco and others added 6 commits November 2, 2024 20:14
@hishamco
Copy link
Member

@sebastienros do you have anything else to add or we can merge, I think this PR takes much time :)

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

@sebastienros sebastienros sebastienros left review comments

@hishamco hishamco hishamco approved these changes

@Piedone Piedone Piedone approved these changes

@gvkries gvkries Awaiting requested review from gvkries

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

In Auto setup, when IsUninitialized, response http status code 409
4 participants
@infofromca @hishamco @Piedone @sebastienros