Skip to content

Provider API value checks should filter out all falsy values, not only None #1883

Closed

Description

Problem

In Provider API scripts, we often check if a required value exists using if <value> is None checks:

foreign_id = image.get("id")
if foreign_id is None:
continue
image_url = image.get("largest_derivative_url")
if image_url is None:
continue

However, this lets other falsy values (such as empty strings) to pass the test, which might cause issues like WordPress/openverse-catalog#1102

Description

Since non-None falsy values (empty strings, empty dictionaries) do not make sense as values for required parameters such as foreign landing url or url, we should replace the is None checks with if value checks.

Alternatives

The current solution with checks for None works in most cases, and the checks could be done in the MediaStore classes. However, it moves the check back in the ingestion process, and makes it longer unnecessarily.

Additional context

The template for provider scripts should be updated, too.

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

Metadata

Assignees

Labels

💻 aspect: codeConcerns the software code in the repository🟩 priority: lowLow priority and doesn't need to be rushed🧰 goal: internal improvementImprovement that benefits maintainers, not users🧱 stack: catalogRelated to the catalog and Airflow DAGs

Type

No type

Projects

  • Status

    ✅ Done

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions