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

Fix issue with namespace rebinding #1800

Conversation

aucampia
Copy link
Member

@aucampia aucampia commented Apr 9, 2022

Also add additional tests for namespace rebinding.
@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

pre-commit.ci autofix

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

@gjhiggins Good spotting on this - I think we need to fix it for other stores also though, I will add an additional test to check this against other stores and then add fixes for that.

@aucampia aucampia mentioned this pull request Apr 9, 2022
@aucampia aucampia requested review from nicholascar and a user April 9, 2022 17:50
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

looks good, thank you

@ghost
Copy link

ghost commented Apr 9, 2022

@gjhiggins Good spotting on this - I think we need to fix it for other stores also though, I will add an additional test to check this against other stores and then add fixes for that.

Maybe save you some time, here's a patch for berkeleydb.py

diff --git a/rdflib/plugins/stores/berkeleydb.py b/rdflib/plugins/stores/berkeleydb.py
index 9ca4380b..857b950d 100644
--- a/rdflib/plugins/stores/berkeleydb.py
+++ b/rdflib/plugins/stores/berkeleydb.py
@@ -470,10 +470,17 @@ class BerkeleyDB(Store):
         prefix = prefix.encode("utf-8")
         namespace = namespace.encode("utf-8")
         bound_prefix = self.__prefix.get(namespace)
-        if override and bound_prefix:
-            self.__namespace.delete(bound_prefix)
-        self.__prefix[namespace] = prefix
-        self.__namespace[prefix] = namespace
+        bound_namespace = self.__namespace.get(prefix)
+        if override:
+            if bound_prefix:
+                self.__namespace.delete(bound_prefix)
+            if bound_namespace:
+                self.__prefix.delete(bound_namespace)
+            self.__prefix[namespace] = prefix
+            self.__namespace[prefix] = namespace
+        else:
+            self.__prefix[bound_namespace or namespace] = bound_prefix or prefix
+            self.__namespace[bound_prefix or prefix] = bound_namespace or namespace
 
     def namespace(self, prefix):
         prefix = prefix.encode("utf-8")

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

@nicholascar @gjhiggins I'm going to throw caution to the wind with this and fix some things which I would have liked to keep to separate PRs in this PR, but we really should try to keep these things less entangled in future so we can get changes in quicker.

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

There are 4 stores doing about the same thing (rdflib.plugins.store.{memory.SimpleMemory,memory.Memory,memory.BerkeleyDB,}), I think the best option is to move the code for Store.{bind,prefix,namespace,namespaces} to a separate class.

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

Will look at this further tomorrow, won't work that well with memory store, but there is also many cases where strings are checked for turthyness instead of is not None, will try fix them also.

@aucampia
Copy link
Member Author

aucampia commented Apr 9, 2022

Actually, scratch the last comments, I will fix further issues and expand tests once this is in master. But beware, there are still at least one store with this issue and then this should be checking is None instead of turthyness, and it should also use a proper null coalescing method, and I'm not entirely sure of all the consequences, but I will figure it out once this and #1686 is merged.

self.__namespace[prefix] = namespace
bound_namespace = self.__namespace.get(prefix)
if override:
if bound_prefix:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if bound_prefix:
if bound_prefix is not None:

if override:
if bound_prefix:
self.__namespace.delete(bound_prefix)
if bound_namespace:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if bound_namespace:
if bound_namespace is not None:

self.__prefix[namespace] = prefix
self.__namespace[prefix] = namespace
else:
self.__prefix[bound_namespace or namespace] = bound_prefix or prefix
Copy link
Member Author

Choose a reason for hiding this comment

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

This will not do the right thing if bound_namespace is ""

self.__namespace[prefix] = namespace
else:
self.__prefix[bound_namespace or namespace] = bound_prefix or prefix
self.__namespace[bound_prefix or prefix] = bound_namespace or namespace
Copy link
Member Author

Choose a reason for hiding this comment

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

This will not do the right thing if bound_prefix is ""

bound_namespace
)
if override:
if bound_prefix:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if bound_prefix:
if bound_prefix is not None:

if override:
if bound_prefix:
del self.__namespace[bound_prefix]
if bound_namespace:
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
if bound_namespace:
if bound_namespace is not None:

self.__prefix[namespace] = prefix
self.__namespace[prefix] = namespace
else:
self.__prefix[bound_namespace or namespace] = bound_prefix or prefix
Copy link
Member Author

Choose a reason for hiding this comment

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

This will not do the right thing if bound_namespace is ""

self.__namespace[prefix] = namespace
else:
self.__prefix[bound_namespace or namespace] = bound_prefix or prefix
self.__namespace[bound_prefix or prefix] = bound_namespace or namespace
Copy link
Member Author

Choose a reason for hiding this comment

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

This will not do the right thing if bound_prefix is ""

Copy link
Member Author

@aucampia aucampia Apr 9, 2022

Choose a reason for hiding this comment

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

Should use null coalesing, something like this:

_AnyT = TypeVar("_AnyT")


def _coalesce(*args: Optional[_AnyT]) -> Optional[_AnyT]:
    for arg in args:
        if arg is not None:
            return arg
    return None

@nicholascar nicholascar deleted the branch RDFLib:default_prefixes April 15, 2022 14:39
@aucampia
Copy link
Member Author

@gjhiggins I will redo this on master.

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