gorilla-co / odata-query

An OData v4 query parser and transpiler for Python
MIT License
71 stars 16 forks source link

alias rewriter: unclear how to specify attributes? #4

Open srepmub opened 3 years ago

srepmub commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

srepmub commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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!

srepmub commented 3 years ago

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 commented 2 years ago

hello oliver,

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

OliverHofkens commented 2 years ago

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 commented 2 years ago

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

OliverHofkens commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 😅