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

Show an error page if libs folder is empty. #20565

Merged
merged 7 commits into from
Aug 22, 2024
Merged

Conversation

maliming
Copy link
Member

@maliming maliming commented Aug 20, 2024

Resolve #20559

The _checkLibsTask will cache the task. It will execute once and won't affect performance.


image

@maliming maliming added this to the 8.3-final milestone Aug 20, 2024
@maliming maliming marked this pull request as ready for review August 20, 2024 06:39
@maliming maliming marked this pull request as draft August 20, 2024 06:40
@maliming maliming marked this pull request as ready for review August 20, 2024 07:39
Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 38.33333% with 37 lines in your changes missing coverage. Please review.

Please upload report for BASE (rel-8.3@d555801). Learn more about missing BASE report.

Files Patch % Lines
.../Volo/Abp/AspNetCore/Mvc/Libs/AbpMvcLibsService.cs 22.91% 37 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             rel-8.3   #20565   +/-   ##
==========================================
  Coverage           ?   52.29%           
==========================================
  Files              ?     3142           
  Lines              ?   100320           
  Branches           ?     7568           
==========================================
  Hits               ?    52467           
  Misses             ?    46279           
  Partials           ?     1574           

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

@ebicoglu
Copy link
Member

Updated the error message

image

protected virtual Task<bool> CheckLibsAsync(HttpContext httpContext)
{
var fileProvider = new PhysicalFileProvider(httpContext.RequestServices.GetRequiredService<IWebHostEnvironment>().WebRootPath);
var libsFolder = fileProvider.GetDirectoryContents("/libs");
Copy link
Member

Choose a reason for hiding this comment

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

If this code is likely to throw an exception, for example if the libs folder cannot be retrieved due to file system permissions, you need to handle the exception.

@maliming maliming requested a review from ebicoglu August 20, 2024 09:52
@ebicoglu ebicoglu merged commit ec43d1b into rel-8.3 Aug 22, 2024
3 checks passed
@ebicoglu ebicoglu deleted the AbpMvcLibsService branch August 22, 2024 06:43
@mtozlu
Copy link
Contributor

mtozlu commented Nov 13, 2024

I throws this exception even if its a /connect request. I think "libs" check should only be applied for non api requests.

@maliming
Copy link
Member Author

hi @mtozlu

The check is only done once during the app's start. So there is no request.

@mtozlu
Copy link
Contributor

mtozlu commented Nov 17, 2024

There's a middleware which suggests it is checked in each request. Am I missing something?
https://github.com/abpframework/abp/pull/20565/files#diff-1e2878883fe3b0732c9e783d6f673c86e49d5c02670696aaf6cb3f0e10b9ceb0R29

I encountered this problem where I get this "libs folder" error page in response to /connect token request.

@maliming
Copy link
Member Author

hi @mtozlu

Sorry, I misremembered.

I will add some checks to skip the Ajax call.

Thanks

@mtozlu
Copy link
Contributor

mtozlu commented Nov 18, 2024

Thanks for the quick addition ^^

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.

4 participants