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

alias rewriter: unclear how to specify attributes? #4

Open
srepmub opened this issue Oct 12, 2021 · 13 comments
Open

alias rewriter: unclear how to specify attributes? #4

srepmub opened this issue Oct 12, 2021 · 13 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@srepmub
Copy link
Contributor

srepmub commented Oct 12, 2021

hi,

odata-query works great for me (using django). except it's not clear to me how to deal with (ugly!) odata attributes. for example I'd like to map Attributes/IntegerAttributes/any(Name eq 'OrbitNumber'??)/{Name, Value} to a literal "OrbitNumber" and a certain db field, respectively. do you have any suggestions on how to proceed..? (an example in the docs would also be great :P)

@srepmub
Copy link
Contributor Author

srepmub commented Oct 15, 2021

to try and be more precise, this seems to more about how to support the following 'any' construct:

$filter=Attributes/OData.CSC.StringAttribute/any(att:att/Name eq 'productType' and att/OData.CSC.StringAttribute/Value eq 'AUX_ECMWFD')

where the actual attributes should be mapped to individual database fields.. so in this case it should just work like:

$filter=productType eq 'AUX_ECMWFD'

@srepmub
Copy link
Contributor Author

srepmub commented Oct 15, 2021

I see there is some level of support for 'any', but when feeding above filter an exception is raised:

(grammar.py, def error)
raise exceptions.TokenizingException(text)
..
NameError: name 'text' is not defined

I guess any python linter would have caught this.. :P

@srepmub
Copy link
Contributor Author

srepmub commented Oct 15, 2021

(problem seems to be the dots, as in OData.CSC.StringAttribute)

@srepmub
Copy link
Contributor Author

srepmub commented Oct 15, 2021

after a removing the dots, the 'any' call is transformed into a nice ast, which I can probably use a rewriter on.. so perhaps the dots are the only issue here.

@srepmub
Copy link
Contributor Author

srepmub commented Oct 20, 2021

from looking at the odata ABNF, the dots are used to indicate 'namespaces'. is there any plan to support these, at least at the ast level?

@OliverHofkens
Copy link
Member

Hey there, glad to hear that the library is (mostly) doing its job for you!
I haven't used OData namespaces yet, but implementing this in the AST sounds like a pretty easy win.
I'm gonna try to reserve some time next friday to try an implementation.

As for the alias rewriter, I agree that the docs are a bit sparse and I'd need to add some examples. In theory it can map any OData construct to something else, but I've personally only used it for simplifying nested relationships, e.g.:
author -> blogpost/author/name.
I'd be happy to look at your case once namespaces are implemented. If the alias rewriter doesn't suffice, a new rewriter that specifically handles OData attributes and namespaces could be an option!

@OliverHofkens OliverHofkens self-assigned this Oct 21, 2021
@OliverHofkens OliverHofkens added documentation Improvements or additions to documentation enhancement New feature or request labels Oct 21, 2021
@srepmub
Copy link
Contributor Author

srepmub commented Oct 22, 2021

thanks! :) yeah, just having the namespaces in the AST would be great. the rewriting part was actually quite easy once I looked better at the existing example.. so that seems fine. also not sure if a generic rewriter would be useful or even possible, as I may be the only one with this particular problem?

@srepmub
Copy link
Contributor Author

srepmub commented Jan 12, 2022

hello oliver,

any updates on namespace support in the AST..? =)

@OliverHofkens
Copy link
Member

I tried adding namespaces in a feature branch about two weeks ago, and it turned out to be harder than I expected (grammar conflicts are a nightmare). Maybe I was too set on the idea that the namespaces should've been integrated as grammar rules. Thinking about it now it would be easier to just allow dots in Identifier nodes, and then post-process that with properties. E.g. OData.CSC.StringAttribute would simply become Identifier('OData.CSC.StringAttribute'), and we can split that in namespace and attribute later.

I'm definitely still periodically working on this, but I can't really give a proper timeline when this will be finished.

@srepmub
Copy link
Contributor Author

srepmub commented Jan 21, 2022

thanks for having a look, and the feedback! we are not blocking on this, but it would be a nice improvement of course.

@OliverHofkens
Copy link
Member

Phew, finally had some time to work on this!
Release 0.5.x now supports parsing namespaces, storing them in the Identifier nodes of the AST.
Nothing is currently done with these namespaces, but at least the parser shouldn't crash.

Note that release 0.5 also includes full typing support, so keep that in mind if you use Mypy and upgrade.

I'm keeping this ticket open to discuss more complete namespace support in the future!

@srepmub
Copy link
Contributor Author

srepmub commented Mar 1, 2022

yeah, thanks! I tried the new version, and it doesn't crash anymore for us, so I removed the regular-expression hack to manually strip namespaces. I'm still wondering though, why I don't see the 'OData.CSC' namespace for this filter:

Attributes/OData.CSC.StringAttribute/any(att:att/Name eq 'processingCenter' and att/OData.CSC.StringAttribute/Value eq 'CLS-Brest')

reflected in the resulting AST:

CollectionLambda(owner=Attribute(owner=Identifier(name='Attributes', namespace=()), attr='StringAttribute'), operator=Any(), lambda_=Lambda(identifier=Identifier(name='att', namespace=()), expression=BoolOp(op=And(), left=Compare(comparator=Eq(), left=Attribute(owner=Identifier(name='att', namespace=()), attr='Name'), right=String(val='processingCenter')), right=Compare(comparator=Eq(), left=Attribute(owner=Attribute(owner=Identifier(name='att', namespace=()), attr='StringAttribute'), attr='Value'), right=String(val='CLS-Brest')))))

I would expect the namespace to appear where it now says "attr='StringAttribute'"..? so eg by changing attr into an Identifier node..?

@OliverHofkens
Copy link
Member

Ah damn, that's an oversight on my part. I didn't think about namespaces appearing in Attributes...
Thanks for reporting back in any case! I think this should be a pretty quick fix, I'll have a look at this soon.

And glad that it doesn't crash anymore in the first place 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants