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

New hardcoded dataserver url points to a 308 Permanent Redirect #131

Merged
merged 1 commit into from
Dec 19, 2023

Conversation

procule
Copy link

@procule procule commented Nov 19, 2023

For some reason (which I guess is my old setup with a lot of customized things), the changed url implemented in 0cc07f7 was not working for me.

After looking at the code and using gdb I found out that the URL with the ẁww. prefix added is a 308 Permanent Redirect to the url without the www. and it was resulting in an unsuccessful request. It may be related to the libsoup version I have installed (2.64.2-2) not following the redirect...

So long story short, removing the www. from the hardcoded URL did the trick and I think that pointing to the real URL would be best.

CURL request:

> GET /cgi-bin/data/dataserver.php HTTP/1.1
> Host: www.aviationweather.gov
> User-Agent: curl/7.73.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 308 Permanent Redirect
< Location: https://aviationweather.gov/cgi-bin/data/dataserver.php

O.

@procule
Copy link
Author

procule commented Nov 19, 2023

Also, according to RFC7538, section 3, The Hypertext Transfer Protocol Status Code 308 (Permanent Redirect):

The 308 (Permanent Redirect) status code indicates that the target
resource has been assigned a new permanent URI and any future
references to this resource ought to use one of the enclosed URIs
.
Clients with link editing capabilities ought to automatically re-link
references to the effective request URI (Section 5.5 of [RFC7230]) to
one or more of the new references sent by the server, where possible.

@procule
Copy link
Author

procule commented Nov 19, 2023

It may be related to the libsoup version I have installed (2.64.2-2) not following the redirect...

Indeed, HTTP 308 Permanent Redirect was added in version 2.71.1 of libsoup:

https://gitlab.gnome.org/GNOME/libsoup/-/commit/51def076e937ce67f97f9a4db6765096b7661673#9f621eb5fd3bcb2fa5c7bd228c9b1ad42edc46c8

@procule
Copy link
Author

procule commented Dec 8, 2023

@raveit65 can this PR be reviewed for merge ?

@procule
Copy link
Author

procule commented Dec 8, 2023

GNOME's libgweather actually used the correct URL without the "www" in their patch: https://gitlab.gnome.org/GNOME/libgweather/-/commit/6c7aa4070b543f41e6f95978464dbef1ce467c41#2e1eaa99e66f946f2e3de5887adbc90fd2d263fc

And in their related issue, the changed url is also mentioned without "www": https://gitlab.gnome.org/GNOME/libgweather/-/issues/232

I guess the added "www" comes from that comment: #79 (comment)

O.

Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

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

Works fine with fedora 38 (libsoup-2.74.3-2.fc38.x86_64).

@raveit65 raveit65 requested review from thesquash and a team December 9, 2023 18:37
Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

A quick test with this installed gave weather information as expected

@procule
Copy link
Author

procule commented Dec 11, 2023

Works fine with fedora 38 (libsoup-2.74.3-2.fc38.x86_64).

A proper test would be with libsoup < 2.71.1 (before their implementation of 308 Permanent Redirect HTTP code)

@bttrx
Copy link

bttrx commented Dec 11, 2023

Works fine with fedora 38 (libsoup-2.74.3-2.fc38.x86_64).

A proper test would be with libsoup < 2.71.1 (before their implementation of 308 Permanent Redirect HTTP code)

I successfully tested billyswong's "no www." binary hack on Linux Mint MATE 20.2 Uma using:
Package: libsoup2.4-1
Version: 2.70.0-1

@raveit65 raveit65 merged commit 3b5440a into mate-desktop:master Dec 19, 2023
1 check passed
@raveit65
Copy link
Member

Works fine with fedora 38 (libsoup-2.74.3-2.fc38.x86_64).

A proper test would be with libsoup < 2.71.1 (before their implementation of 308 Permanent Redirect HTTP code)

I do not use a distro with such an old (obsolete) code base, and i assumed that you tested your PR with an old version, see first post.

New stable release
https://github.com/mate-desktop/libmateweather/releases/tag/v1.26.3

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.

4 participants