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

Move non-http code from FSharp.Data.Http into a new FSharp.Data.Runtime.Utilities project + Rearrange project files #1475

Merged
merged 6 commits into from
Mar 5, 2023

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Feb 26, 2023

closes #1468

For the record, I used the name suggested in the linked issue above for the new common project, but I feel like something like FSharp.Data.Runtime.Common might have been more appropriate.

This is still an improvement over the current situation with everything in FSharp.Data.Http.

I also took the opportunity to rearrange source files into their respective project folders.
Files layout in the src folder should now mostly match the solution organisation.

This also has the nice side effect of making folders reappear in Visual Studio. (for some reason, files included in projects but living outside the project's folder appear as a flat list of linked files in this IDE)

The complete diff is ugly, but reviewing individual commits should be more comfortable.

@mlaily
Copy link
Contributor Author

mlaily commented Feb 26, 2023

The Windows build seems a bit flaky... It's currently failing because it couldn't properly download an rss feed in the docs fsx...

Is there a way to simply retry it?

@mlaily
Copy link
Contributor Author

mlaily commented Mar 5, 2023

@cartermp hey, thanks for taking a look! 🙏

I have a commit ready to shut-up the worldbank api-related test failures.

You have just a word to say and I can add it to the branch of this PR!

(if you want to take a look first: f1115e3)


EDIT: ah, it seems there is another network-related error with the html provider docs -_-
I'll take a look...

EDIT2: The html provider error is caused by the following difference when requesting the https://www.nuget.org/packages/FSharp.Data page 😩

image

mlaily added 2 commits March 5, 2023 18:16
Seeing "see inner exception in the message" without the inner exception
is very frustrating!
@mlaily mlaily force-pushed the rearrange-project-files branch 3 times, most recently from 17fd25d to 7c41bbe Compare March 5, 2023 18:54
@mlaily mlaily force-pushed the rearrange-project-files branch from 7c41bbe to 8c3f4c6 Compare March 5, 2023 19:04
@mlaily
Copy link
Contributor Author

mlaily commented Mar 5, 2023

Well, looks like the last build failure was a TLS version issue...

For the record:

docs\library\XmlProvider.fsx: error(410,11)-(410,51) The type provider 'ProviderImplementation.XmlProvider' reported an error: Cannot read sample XML from 'http://tomasp.net/rss.xml': System.Net.WebException: The SSL connection could not be established, see inner exception.
 ---> System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.
 ---> System.Security.Authentication.AuthenticationException: Authentication failed, see inner exception.
 ---> System.ComponentModel.Win32Exception (0x80090304): The Local Security Authority cannot be contacted
   --- End of inner exception stack trace ---
   at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
   at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptions sslOptions, HttpRequestMessage request, Boolean async, Stream stream, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---

I'm not sure why it would not be enabled on the github windows runner, but adding TLS 1.2 to ServicePointManager.SecurityProtocol in Http.fs fixed the build.
Since it doesn't seem like a very good idea to do that in library code, I enabled it in the design time provider code instead.

All in all, I added four commits to the branch to fix the build.
Sorry about the noise and the notifications you probably received! ^^"

  • The first commit sets the worldbank unit test result to inconclusive when there is an api-related error.
  • The second one sets a less evasive name on the missing Html table (from the HtmlProvider sample) by scraping the aria-label attribute. (This fixes the provided table type changing name randomly in the docs fsx scripts)
  • The third commit reports the complete exception message and stacktrace instead of just the top level exception message when an error occurs while loading a provider sample (which was not very helpful to debug the TLS issue, with the message telling me to look at the inner exception...)
  • The fourth and last commit enables TLS 1.2 in each provider so we are able to request samples from servers that no longer support older versions. (Seems to be the case of https://tomasp.net according to https://www.ssllabs.com/ssltest/analyze.html?d=tomasp.net)

Adding aria-label to the list of html attributes used by the HtmlProvider to infer names is technically a breaking change, but it's also a nice improvement for pages that support it like nuget.org!

I hope this is ok!

@cartermp
Copy link
Collaborator

cartermp commented Mar 5, 2023

Thanks @mlaily. There sure is a lot of ... "legacy behavior"... in here 🙂

@cartermp cartermp merged commit c6f917b into fsprojects:main Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why are the core features (CommonRuntime folder) implemented in the Http project?
2 participants