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

Fixed misleading exception when configuration file is excluded #54717

Closed
wants to merge 1 commit into from
Closed

Fixed misleading exception when configuration file is excluded #54717

wants to merge 1 commit into from

Conversation

YuKitsune
Copy link

@YuKitsune YuKitsune commented Jun 25, 2021

The PhysicalFileProvider.GetFileInfo(string) method currently returns a NotFoundFileInfo if it the file matches any of the provided ExclusionFilter flags. This results in a FileNotFoundException being thrown when trying to load a file using the FileConfigurationProvider.

Example:

System.IO.FileNotFoundException
The configuration file '.env' was not found and is not optional.
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.HandleException(ExceptionDispatchInfo info)
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.Load(Boolean reload)
   at Microsoft.Extensions.Configuration.FileConfigurationProvider.Load()
   at Microsoft.Extensions.Configuration.ConfigurationRoot..ctor(IList`1 providers)
   at Microsoft.Extensions.Configuration.ConfigurationBuilder.Build()

This is misleading as the file may in fact be reachable, but excluded using the ExclusionFilter flags.
I spent some time trying to figure out why my own app couldn't find the .env file I had specified, only to realise after reading the source code for the PhysicalFileProvider that it could have been found if it was not being excluded.

This PR introduces an ExcludedFileInfo and a FileExcludedException to provide better feedback when a file has been intentionally excluded.

The `PhysicalFileProvider.GetFileInfo(string)` method would previously return a `NotFoundFileInfo` if it found a file that matched the provided `ExclusionFilter`.
This is misleading as the file may in fact be reachable, just excluded.
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jun 25, 2021

Tagging subscribers to this area: @maryamariyan, @dotnet/area-extensions-filesystem
See info in area-owners.md if you want to be subscribed.

Issue Details

The PhysicalFileProvider.GetFileInfo(string) method would previously return a NotFoundFileInfo if it found a file that matched the provided ExclusionFilter.
This is misleading as the file may in fact be reachable, just excluded.

Author: YuKitsune
Assignees: -
Labels:

area-Extensions-FileSystem, new-api-needs-documentation

Milestone: -

@dnfadmin
Copy link

dnfadmin commented Jun 25, 2021

CLA assistant check
All CLA requirements met.

@YuKitsune YuKitsune changed the title Added ExcludedFileInfo Fixed misleading exception when configuration file is excluded Jun 25, 2021
@YuKitsune YuKitsune marked this pull request as draft June 25, 2021 06:51
@adamsitnik
Copy link
Member

Hi @YuKitsune

To add a new public API you need to go through the API Review process first.

Once you create a proposal issue the owners of the area take it to the review meeting. After the API gets approved, one can send a PR.

You can read more about it here.

Big thanks for you work and the PR, but for the reasons described above I have to close it until the API gets approved.

@adamsitnik adamsitnik closed this Jun 25, 2021
@SingleAccretion
Copy link
Contributor

SingleAccretion commented Jun 25, 2021

@YuKitsune Maybe, as a simper intermediate fix, the same exception but with a proper message could be thrown?

@YuKitsune
Copy link
Author

Thanks @adamsitnik
My bad, I missed that part when going through the contributing docs.

@SingleAccretion Huh, don't know why I didn't think of that first... That does sound much nicer.
Should I make a proposal issue before making the PR or can I just open up a new PR? I think for a messaging change it should be okay, but I'd just like to double check 🙂

@adamsitnik
Copy link
Member

Should I make a proposal issue before making the PR or can I just open up a new PR? I think for a messaging change it should be okay, but I'd just like to double check

@YuKitsune For just improving an exception message you can open a PR without creating an issue first. If you would like to change the exception type, which would be a breaking change, we would need an issue first.

@YuKitsune
Copy link
Author

@adamsitnik Thanks for clarifying! New PR on the way 🙂

@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants