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

Don't register berkelydb as a store if it is not available on the system #1816

Closed
aucampia opened this issue Apr 14, 2022 · 4 comments · Fixed by #2096
Closed

Don't register berkelydb as a store if it is not available on the system #1816

aucampia opened this issue Apr 14, 2022 · 4 comments · Fixed by #2096
Labels
bug Something isn't working good first issue Good for newcomers store Related to a store.

Comments

@aucampia
Copy link
Member

Currently we are always registering BerkelyDB regardless of whether it is available or not:

rdflib/rdflib/plugin.py

Lines 202 to 207 in cdaee27

register(
"BerkeleyDB",
Store,
"rdflib.plugins.stores.berkeleydb",
"BerkeleyDB",
)

This causes problems if we want to operate on all stores, for example to test some functionality, as a special exception is needed for BerkeleyDB:

def get_store_names() -> Set[Optional[str]]:
names: Set[Optional[str]] = {*get_unique_plugin_names(plugin.Store)}
names.difference_update({
"default",
"Memory",
"Auditable",
"Concurrent",
"SPARQLStore",
"SPARQLUpdateStore",
"SimpleMemory",
})
names.add(None)
if not has_bsddb:
names.remove("BerkeleyDB")
logging.debug("names = %s", names)
return names

I think it would be best to only register the plugin if the store is available.

@aucampia aucampia added the bug Something isn't working label Apr 15, 2022
@aucampia aucampia added the store Related to a store. label Jul 29, 2022
@aucampia aucampia added the good first issue Good for newcomers label Aug 21, 2022
@eden-logistics
Copy link
Contributor

Gonna try working on this, would adding some kind of availability check in the register method work?

@eden-logistics
Copy link
Contributor

Also, I'm assuming the method should check if all plugins are available. Should I do that or just have a specific check for berkelydb?

@aucampia
Copy link
Member Author

Gonna try working on this, would adding some kind of availability check in the register method work?

Also, I'm assuming the method should check if all plugins are available. Should I do that or just have a specific check for berkelydb?

Hi @Jtmcflea I think first and most critical is to just not register plugins we know won't work, sadly there is no generic check we can do for all plugins, we know berkeleydb won't work unless we can import berkeleydb - but berkelydb is really an exception here, not the rule. Most (all?) other stores will work fine with stock python. For other plugins like https://github.com/RDFLib/rdflib-sqlalchemy there may be similar considerations, but that is not something to worry about in this repo.

So I would suggest just do it for berkeleydb.

@eden-logistics
Copy link
Contributor

eden-logistics commented Aug 22, 2022

Ok, I've moved the berkeleydb register to the top of the list and added if rdflib.plugins.stores.berkeleydb.has_bsddb: before it to check if the system has berkeleydb. I've also removed the exception in test_graph.py, which passed, and I'm currently running the validate action in github.

Any more tests or formatting things I might need to do? Sorry to ask, but I'm new at open source contribution in general.

aucampia pushed a commit that referenced this issue Aug 23, 2022
…e system (#2096)

This is so that it is simpler and easier to do something with all plugins without treating different plugins in different ways.

Also remove test code that tries to accommodate the plugin being present but not usable.

Fixes #1816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers store Related to a store.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants