-
Notifications
You must be signed in to change notification settings - Fork 39
feat: Allow in_array expression to fetch all available items - related to #68 #71
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
base: master
Are you sure you want to change the base?
Conversation
I've decided to go with some of the thoughts from last night. So an expression like this This is also way more intuitive because a I had to adjust the regexp for The updated code has tests to check the backward compatibility with I haven't done any performance tests as of now - honestly I'd be hard pressed to see where to squeeze time. Maybe |
Hello @das-peter. First, thanks for the time and effort punt into this feature. I like the idea but, before commiting, I'd like to do some thinking, hopefully we can find the answers together. Before that, though, you expressed confusion about I think this comment is a good starting point to the conversation: #68 (comment)
What I'm thinking of right now is:
I'd like to hear thoughts on this. Let's make sure we implement this the right way! |
@Galbar Thank you so much for getting back on this. It looks like I need to do some more reading.
I'll come back regarding the other things once I feel like I've more qualified ideas. |
For context, this library predates that RFC by many years. If it defines anything that makes this library not-compliant it is most likely that I will ignore it. There were many things in the original spec that were not properly defined and I had to sit down and define them. I stand behind (most of) those decisions. I have not read that RFC fully nor very attentively. While I was writing my previous comment I tried to find in it any reference to the Nevertheless, I think the focus should be in making the feature make sense within the feature-set of this library, hence why my comments revolve around it. The more I think about it, I think the answer of 3. should be that SmartGet gets propagated. I just have the feeling that propagating it properly is going to be a PITA 😅 I'm still unsure about 2. 🤔 |
This is a quick POC related to:
Reason I'd like to see this addition is that some optimized json data schemas normalize the data in order to optimize transport size.
This means references are collected in a meta-data space while the original data structure only contains references to that meta-data space.
A good example is https://jsonapi.org/:
With the extension resolving the detail data of the comments of an entry becomes much easier:
$.included[?(@.id in [$.data[0].relationships.comments.data[*].id])]
There are still some open things in my mind:
$hasDiverged
return value to decide if the result needs to be unwrapped in order to make the above outline scenario and the unit tests work. I'm currently not sure what this does and why I have to do this.And I think the unit test lacks a test case because currently seems to hit only scenarios where
$hasDiverged
set to true.in_array = value 'in' '[' value (',' value)* | jsonpath ']'
Directly allowingjsonpath
conflicts with the "known" limitations of value: "Jsonpaths in value return the first element of the set or false if no result."So this conflicts and most likely also hints that this is a API-Break because the limitation essentially no longer exists for in_array - which can change the behavior of existing expressions - which is bad.
One Idea I could see is that in_array as a more poly-morph syntax - the spec could be:
in_array = value 'in' ('[' value (',' value)* ']' | jsonpath)
$..book[?(@.author in [$.authors[0], $.authors[2]])]
$..book[?(@.author in $.authors)]
Anyhow, it's late here and I'm fighting with falling asleep :)
Feedback would be great but I'll revisit later in any case.