Make User-Agent a default header #3828

krysal · 2024-02-26T18:49:17Z

Fixes

Fixes #1362 by @krysal
Related to #2037 (Airflow work)

Description

Resuming the work done previous to the monorepo. This PR adds the User-Agent as the default header for all requests in the DelayedRequester and ProviderDataIngester classes, making it unnecessary to repeat code in providers DAGs, so it was removed from museum_victoria, nappy, rawpixel, stocksnap, and wikimedia_commons.

I added a new CANONICAL_ORIGIN reading it from the os environment to replace the old domain referenced in the UA string. Is it better to read it from an Airflow variable? CC @AetherUnbound.

Testing Instructions

Run one of the above-mentioned DAGs, for example rawpixel, to confirm they continue to work. See the code and updated tests and confirm it all makes sense.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.
I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

sarayourfriend

LGTM!

sarayourfriend · 2024-02-26T20:35:02Z

catalog/dags/common/loader/provider_details.py

@@ -124,7 +124,8 @@

 # User-Agent header for APIs that require it
 CONTACT_EMAIL = os.getenv("CONTACT_EMAIL")
-UA_STRING = f"Openverse/0.1 (https://wordpress.org/openverse; {CONTACT_EMAIL})"
+CANONICAL_ORIGIN = os.getenv("CANONICAL_ORIGIN", "https://openverse.org")


I guess if we want to match the API's new variables, it should be an environment variable of CANONICAL_DOMAIN with origin derived from it, but I don't know if that was a goal with this PR. It doesn't matter to me either way, as there's so little overlap between the variables for each I don't think we need to be concerned with having them match except when the names are this similar, it would be easier to make a quick mistake.

To clarify, not a suggestion for a change, just noting the slight difference, will at least help me keep it in mind during infra work to migrate Airflow's environment variables to the new approach.

Yes, I intended to make the change aligned with your on moving the domain :) I thought the CANONICAL_DOMAIN was a variable we could skip here, but you're right. It would be better to match the API's variables in this case.

Done ✅

stacimc · 2024-02-26T21:14:34Z

catalog/dags/common/requester.py

        self._DELAY = delay
-        self.headers = headers or {}
+        self.headers = {"User-Agent": prov.UA_STRING} | headers


Why do we default here in addition to adding this as a default in the ProviderDataIngester?

That was the suggestion of the issue, and I went that route first, but it turns out it wasn't enough to set the default header. I leave it after updating the ProviderDataIngester as it will be better if each request from the Calalog goes with this header.

😮 That's so strange -- do you mean setting it as the default in DelayedRequester did not cover all the existing cases? Are we making requests that don't use the requester somewhere?

If it can be added in only one place, I do agree about adding it in the requester instead of the ProviderDataIngester, since that gets complicated with subclasses that override __init__ etc.

do you mean setting it as the default in DelayedRequester did not cover all the existing cases?

Yes, the thing is the ProviderDataIngester class passes each time its headers (previously an empty dictionary) to the DelayedRequester.get_response_json method (for the possibility of overriding them), so it wasn't enough to add them to the DelayedRequester only.

Are we making requests that don't use the requester somewhere?

It's a possibility, I haven't checked but this isn't the issue here, as I explained above. We should be safe with these changes!

stacimc

Couple of questions but nothing blocking. LGTM and all DAGs work as expected 🚀

stacimc · 2024-02-26T21:24:13Z

catalog/dags/common/loader/provider_details.py

+CANONICAL_DOMAIN: str = os.getenv("CANONICAL_DOMAIN", "openverse.org")
+
+_proto = "http" if "localhost" in CANONICAL_DOMAIN else "https"
+CANONICAL_ORIGIN: str = f"{_proto}://{CANONICAL_DOMAIN}"


Is this just for consistency with the api? Would we ever actually use a domain with "localhost" in this context?

Yes, this arose from my conversation with Sara here. We opted for keeping consistency with the API in any case.

stacimc · 2024-02-26T21:30:23Z

catalog/dags/common/requester.py

        self._DELAY = delay
-        self.headers = headers or {}
+        self.headers = {"User-Agent": prov.UA_STRING} | headers


😮 That's so strange -- do you mean setting it as the default in DelayedRequester did not cover all the existing cases? Are we making requests that don't use the requester somewhere?

If it can be added in only one place, I do agree about adding it in the requester instead of the ProviderDataIngester, since that gets complicated with subclasses that override __init__ etc.

krysal added 🟩 priority: low Low priority and doesn't need to be rushed ✨ goal: improvement Improvement to an existing user-facing feature 💻 aspect: code Concerns the software code in the repository 🧱 stack: catalog Related to the catalog and Airflow DAGs labels Feb 26, 2024

krysal requested a review from a team as a code owner February 26, 2024 18:49

krysal requested review from obulat and stacimc February 26, 2024 18:49

openverse-bot added the 🕹 aspect: interface Concerns end-users' experience with the software label Feb 26, 2024

sarayourfriend approved these changes Feb 26, 2024

View reviewed changes

krysal added 5 commits February 26, 2024 16:57

Add User-Agent as default header to DelayedRequester

5f5f55f

Remove User-Agent header form provider DAGs

5c32dcf

Test User-Agent in ProviderDataIngester class

b7ab1f8

Set User-Agent header in ProviderDataIngester

db8fe6b

Update User-Agent string with CANONICAL_ORIGIN

ad6b3da

krysal force-pushed the update/ua_string branch 2 times, most recently from 5412660 to b8eccfb Compare February 26, 2024 21:05

stacimc reviewed Feb 26, 2024

View reviewed changes

Add CANONICAL_DOMAIN variable and derive CANONICAL_ORIGIN from it

c4c7f39

krysal force-pushed the update/ua_string branch from b8eccfb to c4c7f39 Compare February 26, 2024 21:19

stacimc approved these changes Feb 26, 2024

View reviewed changes

krysal merged commit 6636dcf into main Feb 28, 2024
43 checks passed

krysal deleted the update/ua_string branch February 28, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make User-Agent a default header #3828

Make User-Agent a default header #3828

krysal commented Feb 26, 2024

sarayourfriend left a comment

sarayourfriend Feb 26, 2024

krysal Feb 26, 2024

stacimc Feb 26, 2024

krysal Feb 26, 2024

stacimc Feb 26, 2024

krysal Feb 26, 2024 •

edited

Loading

stacimc left a comment

stacimc Feb 26, 2024

krysal Feb 26, 2024

stacimc Feb 26, 2024

Make User-Agent a default header #3828

Make User-Agent a default header #3828

Conversation

krysal commented Feb 26, 2024

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krysal Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

stacimc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krysal Feb 26, 2024 •

edited

Loading