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.

Sign up for GitHub

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

Jump to bottom

Factor out DatastoreResource class #4372

Open
wants to merge 23 commits into
base: 2.x
Choose a base branch
from
Open

Factor out DatastoreResource class #4372

wants to merge 23 commits into from

Conversation

dafeder
Copy link
Member

@dafeder dafeder commented Dec 27, 2024 •
edited
Loading

Fixes #4359

Describe your changes

Remove DatastoreResource class, use DataResource for everything. Note that any class or function that expected a DatastoreResource object now expects a DataResource object. So any custom code that, for instance, builds a datastore DatabaseTable object will need to be slightly refactored.

Other changes to note:

  • AbstractDatabaseTable::setTable() is now public. We should use this directly instead of calling ::count() (as we do currently in some places) to indirectly create a database table.
  • DictionaryEnforcer now requires the dkan.datastore.database_table_factory service

Please override the CC warnings when reviewing, they are due to CC getting confused by the ReturnTypeMayChange attribute. May be an issue with CC config.

QA Steps

Nothing to QA, everything should still work as it did before from user's perspective.

Checklist before requesting review

If any of these are left unchecked, please provide an explanation

  • I have updated or added tests to cover my code
  • I have updated or added documentation

@dafeder
Copy link
Member Author

dafeder commented Dec 27, 2024 •
edited
Loading

Currently debugging failing MySqlQueryDownloadControllerTest, which I think just happens to be the first test to run that tries to run a query. It fails at AbstractDatabaseTable::setTable(), I think something is making the perspective be local_file when it should be something else? It can't get a schema anyway.

@dafeder dafeder changed the title Factor our DatastoreResource class Factor out DatastoreResource class Dec 27, 2024
@dafeder
Copy link
Member Author

dafeder commented Dec 30, 2024

Some progress. Need to redo QueryDownloadControllerTest somehow... not so easy to use fake resource names because table name is no longer based on an ID in the DatastoreResource class that we can simply control via the constructor.

@dafeder dafeder marked this pull request as ready for review January 7, 2025 20:42
@dafeder dafeder requested review from paul-m and kaise-lafrai January 7, 2025 20:43
paul-m
paul-m requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@paul-m paul-m left a comment

Choose a reason for hiding this comment

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

Drupal\Tests\datastore\Unit\Storage\DatabaseTableFactoryTest still creates a DatastoreResource.

paul-m
paul-m requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@paul-m paul-m left a comment

Choose a reason for hiding this comment

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

Let's add to the docblock of Drupal\datastore\Storage\DatabaseTableFactory::getInstance() that the resource must be a DataResource object.

paul-m
paul-m requested changes Jan 7, 2025
View reviewed changes
Copy link
Contributor

@paul-m paul-m left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Drupal\Tests\datastore\Unit\Plugin\QueueWorker\ImportJobTest spends a lot of time making DatastoreResource objects.

Drupal\Tests\datastore\Unit\Service\Info\ImportInfoTest too.

Basically search the codebase for DatastoreResource and you'll see a bunch of tests.

@dafeder
Copy link
Member Author

dafeder commented Jan 8, 2025

Let's add to the docblock of Drupal\datastore\Storage\DatabaseTableFactory::getInstance() that the resource must be a DataResource object.

@paul-m see 2594e68

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

@paul-m paul-m paul-m approved these changes

@kaise-lafrai kaise-lafrai Awaiting requested review from kaise-lafrai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consolidate DataResource and DatastoreResource
2 participants
@dafeder @paul-m