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

Clean Datastore Tables Job #67

Conversation

JVickery-TBS
Copy link

feat(jobs): added job to delete datastore tables;

  • Added config option clean_datastore_tables.
  • Added job to delete datastore tables from unsupported formats.

This adds the possible feature (controlled by config option) to delete datastore tables for unsupported formats. This is useful in the case that a user updates a Resource to no longer be a datastore resource, but the tables remain. So this helps keep the size of the datastore database down a bit

See: ckan#196

- Added config option `clean_datastore_tables`.
- Added job to delete datastore tables from unsupported formats.
…ported-datastore-tables

# Conflicts:
#	requirements.txt
### RESOLVED.
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
- Used `get_action` from toolkit instead of ckanapi dependency.
- Reworked some coe to make it more gooder.
- Removed `url_changed` check.
- Removed `ckanapi` import.
- Better logging.
- Added `asbool` for `datastore_active` checks.
@JVickery-TBS
Copy link
Author

@ThrawnCA did feedback from ckan#196 here too

ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
@duttonw duttonw changed the base branch from master to develop November 10, 2023 05:43
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
- Added falsy check for `url_type`.
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/plugin.py Outdated Show resolved Hide resolved
- Removed `lower` call in logging.
- Fixed falsy check on `url_type`.
@JVickery-TBS
Copy link
Author

@ThrawnCA .lower() also removed from the debug logging on resource format

Copy link
Member

@duttonw duttonw left a comment

Choose a reason for hiding this comment

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

It does seem like this should be working code, but we all would be more confident if some tests were added.

Reviewing what you are wanting. I was thought experimenting if this logic could live inside the 'ckanext-datastore' since its about 'cleanup' of said data stores. Sadly a quick look through said codebase, it does not cover the hooks required for this nor has the ability to know what formats we allow to ingest in a common bases between datapusher/xloader.

ckanext/xloader/plugin.py Show resolved Hide resolved
@duttonw
Copy link
Member

duttonw commented Nov 15, 2023

This might help in getting your discrete tests on this function without needing to also test the xloader 'is file supported' function.

@pytest.fixture
def mock_toolkit_config(request):
    with mock.patch('ckan.plugins.toolkit.config.get') as mock_get:
        mock_get.return_value = request.param
        yield mock_get

@pytest.fixture
def mock_xloader_formats(request):
    with mock.patch('your_module.XLoaderFormats.is_it_an_xloader_format') as mock_is_xloader_format:
        mock_is_xloader_format.return_value = request.param
        yield mock_is_xloader_format


@pytest.mark.parametrize("toolkit_config_value, xloader_formats_value, expected_result", [
    (True, True, True),
    (True, False, False), 
    (False, True, False),
    (False, False, False),
]) <!-- this allows you to test all the combinations with less code, you might also want to pass in the url_type/datastore_active flags also OR you can go with individual discrete tests with nice names-->
def test_should_remove_unsupported_resource_from_datastore(
    mock_toolkit_config, mock_xloader_formats, toolkit_config_value, xloader_formats_value, expected_result
):
    # Setup mock data
    res_dict = {
        'format': 'some_format',
        'url_type': 'upload',
        'datastore_active': True,
        'extras': {'datastore_active': True}
    }

    # Call the function
    result = _should_remove_unsupported_resource_from_datastore(res_dict)

    # Assert the result based on the logic paths covered
    assert result == expected_result
    ```

Develop to master - add button to delete datastore table
Develop to master - restrict lock timeout to prevent deadlocks
Develop to master - handle lock conflicts more robustly
- Syntax fixes from flake8.
…ported-datastore-tables

# Conflicts:
#	ckanext/xloader/plugin.py
### RESOLVED.
- Added new test for should remove unsupported resources from datastore.
@JVickery-TBS
Copy link
Author

@duttonw @ThrawnCA okay got those automated tests in now. Thanks so much for giving me the code! My python knowledge would have never gotten there hahaha.

I did add in the url_type and datastore_active params

As for the greedy-ness of this functionality, I think it is kind of greedy in general to trash datastore tables if the Resource Format changes. But users can always just re-upload to the datastore if they need to

ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
ckanext/xloader/tests/test_plugin.py Outdated Show resolved Hide resolved
- Finalized fixtures and params for new test method.
@JVickery-TBS
Copy link
Author

@ThrawnCA @duttonw okay I think the tests should be good now. I had to play around with the fixtures a bit, and seems like we do need the yields in the fixtures if your patching class methods

ThrawnCA and others added 2 commits February 2, 2024 12:34
- Also move the test case documentation alongside the test data so cases can be added or removed in just one place
…ported-datastore-tables

replace complex indirect fixtures with simple 'with' blocks
@duttonw duttonw merged commit ee5760b into qld-gov-au:develop Feb 3, 2024
1 check passed
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.

3 participants