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

Handle not supported content encoding #327

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

y-code
Copy link

@y-code y-code commented Sep 7, 2019

Although HtmlWeb class allows to ignore resp.ContentEncoding by OverrideEncoding property, it cannot help in the case where resp.ContentEncoding has encoding name which is not supported by Encoding class, and throws ArgumentException.

When OverrideEncoding is provided, the result from Encoding.GetEncoding method has not been actually used so that simply skipping Encoding.GetEncoding method call can make it work without impact to existing behaviors.

EncodingNotSupportedException is added in order to notify that such not supported content encoding is received from server. It helps when writing code to retry loading page with OverrideEncoding.

fix #326

… encoding

Although HtmlWeb class allows to ignore resp.ContentEncoding by OverrideEncoding property, it cannot help in the case where resp.ContentEncoding has encoding name which is not supported by Encoding class. This is because Encoding.GetEncoding method throws ArgumentException in that case.

When OverrideEncoding is provided, the result from resp.ContentEncoding has not been used, so that simply skipping Encoding.GetEncoding method call can make it work.

EncodingNotSupportedException is added in order to notify that such not supported content encoding is received in the response, with the information of the received content encoding name. This is more helpful than ArgumentException.
…ible in public

modify access modifier of Encoding property of EncodingNotSupportedException from internal to public.
@JonathanMagnan JonathanMagnan self-assigned this Sep 9, 2019
@JonathanMagnan
Copy link
Member

Hello @y-code ,

Thank you for the pull, we will look at it.

Best Regards,

Jonathan


Performance Libraries
context.BulkInsert(list, options => options.BatchSize = 1000);
Entity Framework ExtensionsEntity Framework ClassicBulk OperationsDapper Plus

Runtime Evaluation
Eval.Execute("x + y", new {x = 1, y = 2}); // return 3
C# Eval FunctionSQL Eval Function

@JonathanMagnan
Copy link
Member

Hello @y-code ,

Sorry for the long delay, we were unable to work on HAP during September.

We checked the pull and it looks good but it's possible to get one case in which the new exception will be throw?

If you want, you can add it to the unit test section or just provide the code here.

@y-code
Copy link
Author

y-code commented Oct 6, 2019

Hi @JonathanMagnan,

Thank you for having time to review my change. I'll try to add a unit test.

@JonathanMagnan
Copy link
Member

Hello @y-code ,

Do you think you would have time to add those unit tests soon?

We currently cannot merge this fix since we don't know how to reproduce the issue.

@JonathanMagnan
Copy link
Member

Hello @y-code ,

Whenever you are ready to provide a unit test, just add it and we will re-open this pull.

Meanwhile, we will close it as my developer asked me to don't push it unless we better understand what will be the impact and for this, we need an example.

wrap HttpRequest and HttpResponse with interfaces, and make HtmlWeb class testable.
add a test project targeting multiple .NET version.
copy the existing tests in HtmlAgilityPack.Tests.Net45.
add test code generator
add test code for HtmlWeb.Load method
It turned out NUnit tests are not running with core 2.0. Therefore, changed test project target to core 2.1 so that we can run tests over .NET Core.
add mshome.html file load to mock in HtmlWebTests.TestLoad.
modify how to get assembly location in generated test code in HtmlWebTestGenerator.
The code was never be active because of the wrong condition and wrong symbols in the processor directive.

correct it so that the code is activated in net4.5 and newer.
add a preprocessor symbol in two projects so as to build HtmlAgilityPack.Master solution also with Debug configuration.
@y-code
Copy link
Author

y-code commented Nov 16, 2019

Hi @JonathanMagnan,

I added tests for my change. Please re-open this pull request and review the code I added.

In order to make HtmlWeb class testable in unit test, I added a code refactoring in the class. I hope you wouldn't mind this change. This refactoring abstracts HttpWebRequest and HttpWebResponse used in HtmlWeb class. It applies only to the code active with preprocessor symbol !(NETSTANDARD1_3 || NETSTANDARD1_6 || METRO).

For testing, I added another project so as to run the tests over .NET Core as well. This project targets multiple frameworks; net40, net45 and netcoreapp2.1. The tests can be run with every framework in Visual Studio. To run them in the command line, dotnet.exe test can run tests only over netcoreapp2.1. For net40 and net45, we still need to use nunit3-console.exe. To debug test with netcoreapp2.1, it sounds not reasonable but we need to edit csproj file so that netcoreapp2.1 comes first in tag.

I also migrated the existing tests in HtmlAgilityPack.Tests.Net45 test project to the new test project, so if the test project looks okay, we can remove HtmlAgilityPack.Tests.Net45 test project.

In addition, I added one more project for a tool to generate test code. It was a code I used for creating the base of the test code which I added this time. I thought it could be generally useful, so I included it in this pull request. If it is not, please advise me to remove it.

@JonathanMagnan
Copy link
Member

Awesome,

We will look at it ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

override encoding cannot help when receiving not supported context encoding
2 participants