Skip to content

Conversation

@jdesboeufs
Copy link
Member

Summary

Makes the addok-sqlite-store plugin non-intrusive by removing automatic activation when installed. The plugin now requires explicit configuration to be used.

Problem

Previously, simply installing addok-sqlite-store would automatically set it as the document store backend via the preconfigure() function. This caused issues in environments where:

  • Multiple storage backends are installed (e.g., in Docker images)
  • Redis or another backend should be used by default
  • Users want explicit control over which backend is active

Changes

  • Removed DOCUMENT_STORE_PYPATH from preconfigure(): The plugin no longer auto-activates itself
  • Kept SQLITE_DB_PATH in preconfigure(): Still provides a sensible default (addok.db) for the database path
  • Updated README: Added clear instructions showing that users must explicitly set DOCUMENT_STORE_PYPATH to activate the plugin

Migration Guide

After this change, users need to explicitly configure the plugin:

# In your Addok configuration file
DOCUMENT_STORE_PYPATH = 'addok_sqlite_store.SQLiteStore'

Or via environment variable:

export ADDOK_DOCUMENT_STORE_PYPATH='addok_sqlite_store.SQLiteStore'

The SQLITE_DB_PATH configuration remains optional with a default value of addok.db.

Benefits

  • ✅ Plugin discovery still works via entry points
  • ✅ No unexpected behavior changes when installing the plugin
  • ✅ Compatible with multi-backend Docker images
  • ✅ Explicit configuration makes dependencies clear
  • ✅ Tests already followed this pattern (no test changes needed)

- Remove DOCUMENT_STORE_PYPATH from preconfigure()
- Keep SQLITE_DB_PATH default value in preconfigure()
- Update README to clarify plugin must be explicitly activated
- Plugin now requires explicit configuration to be used
@jdesboeufs jdesboeufs requested a review from Copilot November 1, 2025 22:43
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the plugin from automatically configuring itself to requiring manual configuration by users. The key modification removes the automatic registration of DOCUMENT_STORE_PYPATH from the preconfigure function, making users explicitly set this configuration parameter.

  • Removed automatic DOCUMENT_STORE_PYPATH setting in the preconfigure function
  • Updated documentation to reflect the manual configuration requirement
  • Removed claims about "automatic registration" from feature list

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
addok_sqlite_store/init.py Removed automatic setting of DOCUMENT_STORE_PYPATH in preconfigure function
README.md Updated documentation to instruct users to manually configure DOCUMENT_STORE_PYPATH, added configuration examples
Comments suppressed due to low confidence (1)

addok_sqlite_store/init.py:14

  • The environment variable lookup is checking for 'SQLITE_DB_PATH', but according to the updated README, the environment variable should be 'ADDOK_SQLITE_DB_PATH'. This mismatch will cause the environment variable configuration method documented in the README to not work.
        self.conn = sqlite3.connect(os.environ.get('SQLITE_DB_PATH') or config.SQLITE_DB_PATH)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- ADDOK_SQLITE_DB_PATH is the recommended convention
- SQLITE_DB_PATH is kept for backward compatibility
- Priority: ADDOK_SQLITE_DB_PATH > SQLITE_DB_PATH > config.SQLITE_DB_PATH
@jdesboeufs jdesboeufs requested a review from Copilot November 1, 2025 22:50
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

addok_sqlite_store/init.py:51

  • The flushdb method uses config.SQLITE_DB_PATH directly, which doesn't respect the environment variable priority chain established in the init method. This means flushdb may attempt to delete a different database file than the one currently being used. Store the resolved db_path as an instance variable in init (e.g., self.db_path) and use it here instead.
    def flushdb(self):
        os.unlink(config.SQLITE_DB_PATH)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Store db_path as instance variable to ensure flushdb deletes
the actual database file being used, not just config.SQLITE_DB_PATH
@jdesboeufs jdesboeufs merged commit 654faa1 into main Nov 1, 2025
2 checks passed
@jdesboeufs jdesboeufs deleted the non-intrusive branch November 1, 2025 22:54
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.

2 participants