Clarify/Refactor Archives Logic

[maxachis]

The Archives resource and its attendant functionality is unclear to me on several levels:

1. I do not know where its endpoint is being called from
2. I do not fully understand under what conditions a data source is to be archived
3. The logic for determining what is archived seems awkward, and I'm not sure why a simple flag of "archived" in each row would not suffice:

def archives_get_results(conn: PgConnection) -> list[tuple[Any, ...]]:
    """
    Pulls data sources for the automatic archives script that performs caching

    :param conn: A psycopg2 connection object to a PostgreSQL database.
    :return: A list of dictionaries representing the rows matching the query conditions.
    """
    cursor = conn.cursor()
    sql_query = """
    SELECT
        airtable_uid,
        source_url,
        update_frequency,
        last_cached,
        broken_source_url_as_of
    FROM
        data_sources
    WHERE 
        (last_cached IS NULL OR update_frequency IS NOT NULL) AND broken_source_url_as_of IS NULL AND url_status <> 'broken' AND source_url IS NOT NULL
    """
    cursor.execute(sql_query)

    return cursor.fetchall()

I'd like to figure out the answer to these questions, and possibly refactor the Archives resource for improved clarity, or update the documentation.

related to Police-Data-Accessibility-Project/automatic-archives#21

[josh-chamberlain]

@maxachis I think this is coming from/used by code in the automatic-archives repo: https://github.com/Police-Data-Accessibility-Project/automatic-archives

[maxachis]

@josh-chamberlain Related to what is discussed in this issue, I think setting up an archived_data_sources table in the database will help clarify logic and keep data_sources from being too populated.

Also (and possibly relevant to the above point), I would like to know by what conditions we determine whether a data source should be archived, or if every data source should be archived.

[EvilDrPurple]

@maxachis

Also (and possibly relevant to the above point), I would like to know by what conditions we determine whether a data source should be archived, or if every data source should be archived.

If I'm following the archive logic correctly in the SQL query, in plain english:
A source is archived if:
The source was never archived before OR the source is updated regularly
AND the source url is not broken
AND the source url is not null

So we basically don't want to waste time archiving sources that are never updated and have already been archived or are broken

[josh-chamberlain]

@EvilDrPurple nailed it! want to work on this issue / the other one linked? this is not related to v2, but it's still pretty important that it works correctly and clearly. One day we will get more sophisticated than internet archive.

@maxachis agreed. it was built by someone without a ton of experience, so it makes sense that there would be room for improvement and clarity! at most, data_sources would be linked to archived_data_sources so we know which sources we're talking about, and can answer questions about when a source was last archived and stuff.

I'd consider renaming this issue to something like "separate archives logic from data sources app". Ideally, our API would just exist, and automatic-archives could do everything it needs to do with the API. automatic-archives is an example of the kind of project we would love for other people to build ~independently using our API.

[EvilDrPurple]

@EvilDrPurple nailed it! want to work on this issue / the other one linked? this is not related to v2, but it's still pretty important that it works correctly and clearly. One day we will get more sophisticated than internet archive.

@josh-chamberlain yeah sure! I believe I may need access to the database but I can start on it

[EvilDrPurple]

(last_cached IS NULL OR update_frequency IS NOT NULL) AND broken_source_url_as_of IS NULL AND url_status <> 'broken' AND source_url IS NOT NULL

After looking at this line again, do you think it would be better to have a database-side function with this logic inside it, that way the query would call is_archiveable() or something like it that returns True or False? Or do you think this would obfuscate the logic of the query too much?

[maxachis]

(last_cached IS NULL OR update_frequency IS NOT NULL) AND broken_source_url_as_of IS NULL AND url_status <> 'broken' AND source_url IS NOT NULL

After looking at this line again, do you think it would be better to have a database-side function with this logic inside it, that way the query would call is_archiveable() or something like it that returns True or False? Or do you think this would obfuscate the logic of the query too much?

@EvilDrPurple This has in fact been an ongoing discussion that I and @josh-chamberlain have been having, which is the degree to which we want to have database functions versus not. While on the one hand adding them helps keep SQL-side code away from the app and more tightly relates it to the database functionality, it also means it is instead located in the database, where visibility is not as strong and we lack strong version control, at least at the moment.

dev_scripts.sql provides us a limited form of version control, which does at least allow us to retrace our steps. And in my work on #362 , I've developed a view to consolidate some of the more common joins, which is not that far removed from a function.

Josh can correct me on this, but my view is that until we solidify standards, we may want to operate on a case-by-case basis.

In this case, my question is whether this logic is (or is likely to be) re-used. If yes, then a function or a view would be appropriate. Otherwise, we're just adding a layer of abstraction to a query that requires a person to look in both the app and the database for the full picture.

I leave it up to you to determine which scenario we're in 😈!

[EvilDrPurple]

In this case, my question is whether this logic is (or is likely to be) re-used. If yes, then a function or a view would be appropriate. Otherwise, we're just adding a layer of abstraction to a query that requires a person to look in both the app and the database for the full picture.

I don't believe it is likely to be reused or already is. You make good points!

[EvilDrPurple]

What could be done is making it so that next_cache is only set when a url is to be archived and having next_cache be set to a value hinge on the other pre-conditions in this query 🤔

[maxachis]

What could be done is making it so that next_cache is only set when a url is to be archived and having next_cache be set to a value hinge on the other pre-conditions in this query 🤔

I'm interested in this proposal and curious to see it expanded upon! 🧐

[EvilDrPurple]

@maxachis Yeah, I'm not sure how realistic it is depending on how many places have potential updates to these values, but one easy point would be when we update the url_status to broken we set next_cache to null at the same time. That way we can safely strip that part from the query since we know all broken urls should have it set to null. This raises some difficulties, like determining when to set next_cache again if the source url is fixed later