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

Query constructor for grouped models injects field name instead of SPARQL binding #161

Closed
lu-pl opened this issue Dec 4, 2024 · 2 comments · Fixed by #162 · May be fixed by #132
Closed

Query constructor for grouped models injects field name instead of SPARQL binding #161

lu-pl opened this issue Dec 4, 2024 · 2 comments · Fixed by #162 · May be fixed by #132
Assignees
Labels
bug Something isn't working

Comments

@lu-pl
Copy link
Contributor

lu-pl commented Dec 4, 2024

Apparently, the query constructor for grouped models injects the field name instead of the SPARQL binding name when rdfproxy.SPARQBinding aliasing is used.

This is almost certainly connected with the change to model field grouping, the aliasing is handled in the mapper but also needs handling in the query constructor.

@lu-pl lu-pl added the bug Something isn't working label Dec 4, 2024
@lu-pl lu-pl self-assigned this Dec 4, 2024
@lu-pl lu-pl changed the title Query constructor for grouped models injects field name instead of SPARQL bindings Query constructor for grouped models injects field name instead of SPARQL binding Dec 4, 2024
@lu-pl
Copy link
Contributor Author

lu-pl commented Dec 4, 2024

rdfproxy.utils.sparql_utils.get_items_query_constructor likely is the culprit, it naively passes on the group_by value from the model.

This is clearly a design flaw, there should be a single interface for resolving aliases -> refactor todo!

@lu-pl
Copy link
Contributor Author

lu-pl commented Dec 4, 2024

class FieldsBindingsMap(UserDict):
    def __init__(self, model: type[_TModelInstance]):
        self.data = self._get_field_binding_mapping(model)

    @staticmethod
    def _get_field_binding_mapping(model: type[_TModelInstance]) -> dict[str, str]:
        return {
            k: next(filter(lambda x: isinstance(x, SPARQLBinding), v.metadata), k)
            for k, v in model.model_fields.items()
        }

lu-pl added a commit that referenced this issue Dec 5, 2024
Currently, query constructors naively inject the 'group_by' value from
the model_config into SPARQL queries; this works until a field name does not correspond to a SPARQL binding name i.e. until rdfproxy.SPARQLBinding aliases are used.

So, field-based grouping obviously requires 'group_by' values to
resolve SPARQL binding aliases - which this change implements.

Note: The change introduces duplication for resolving SPARQL binding
aliases. The planned QueryConstructor class (issue #134) will get rid of
this and other design flaws.

Fixes #161
lu-pl added a commit that referenced this issue Dec 5, 2024
Currently, query constructors naively inject the 'group_by' value from
the model_config into SPARQL queries.
This works until a field name does not correspond to a SPARQL binding
name, i.e. until rdfproxy.SPARQLBinding aliases are used.

So, field-based grouping obviously requires 'group_by' values to
resolve SPARQL binding aliases - which this change implements.

Note: The change introduces duplication for resolving SPARQL binding
aliases. The planned QueryConstructor class (issue #134) will get rid of
this and other design flaws.

Fixes #161
kevinstadler pushed a commit that referenced this issue Dec 5, 2024
Currently, query constructors naively inject the 'group_by' value from
the model_config into SPARQL queries.
This works until a field name does not correspond to a SPARQL binding
name, i.e. until rdfproxy.SPARQLBinding aliases are used.

So, field-based grouping obviously requires 'group_by' values to
resolve SPARQL binding aliases - which this change implements.

Note: The change introduces duplication for resolving SPARQL binding
aliases. The planned QueryConstructor class (issue #134) will get rid of
this and other design flaws.

Fixes #161
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant