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

Nesting of aql templates #481

Closed
Nainterceptor opened this issue Dec 4, 2017 · 13 comments
Closed

Nesting of aql templates #481

Nainterceptor opened this issue Dec 4, 2017 · 13 comments
Assignees
Labels
Feature Request Request for new functionality to be added to the driver. semver-minor This issue does not require any backwards-incompatible changes to address.

Comments

@Nainterceptor
Copy link

Hello,

For complexe queries, aql templates seems to be a bit tricky to use. Maybe the best way is to use query builder. But, I think recursivity may help.

For exemple :

query(aql`
    FOR v in ${array.map(c => aql`DOCUMENT(${c.id})`)}
      FILTER NOT_NULL(v)
      # Some other funny things here
    return v
    `);

Or maybe the simplest exemple :

query(aql`
    FOR v in [${aql`DOCUMENT(${'foo/bar'})`}]
    return v
    `);

Is it a good idea ? Probably not. But, may help in some cases like mine.
For now, I've done the job without aql template, but the result may be less safe (here, id are not from user input)

Have a nice day,
Gaël

@pluma
Copy link
Contributor

pluma commented Dec 4, 2017

Currently queries are not composable, yes. This is a known limitation because merging bindVars is nontrivial.

Supporting this would likely require scanning the argument list for AQL objects and prepopulating the bindVars so the numbers remain consistent. However this suggest we should also mark the AQL objects so we know they're actual AQL objects and can be trusted (unlike user input).

I'm not 100% sure this is a good idea but composability beyond simple {toAQL() {return 'LIMIT whatever'}} (which we already support) seems like a logical progression.

@pluma pluma changed the title Recursive usage of aql templates Nesting of aql templates Dec 4, 2017
@Brian-McBride
Copy link

Wow, I was just looking for something like this.
I have a number of use cases where I need to compose a query based on dynamic input. It might be as simple as having multiple FILTER lines that may or may not exist. I could have my microservice reference a library of queries for each use case, but it seems that a map of my source to extend my aql query is ideal.

@pluma pluma added Feature Request Request for new functionality to be added to the driver. semver-minor This issue does not require any backwards-incompatible changes to address. labels Dec 19, 2017
@pluma
Copy link
Contributor

pluma commented Dec 19, 2017

We're investigating a possible implementation of this, which may land in the upcoming 6.0.0 release.

@dsonet
Copy link

dsonet commented Jan 2, 2018

Ya this is exactly what we need. How is the progress?
Thanks

@pluma
Copy link
Contributor

pluma commented Jan 3, 2018

It's more complicated than initially anticipated. The problem is that nesting aql expressions means the bindvars have to be renamed recursively, before the partial queries (and bindvar objects) can be merged. This gets even more complicated when attempting to deduplicate the bindvars (which we currently do). Right now the return value consists of just the query and the bindvars, to do all this we need to extend that a fair bit.

I'm scheduled to be back in office next week, so I'll look into it again then.

@pluma pluma self-assigned this Jan 3, 2018
@pluma
Copy link
Contributor

pluma commented Mar 6, 2018

Just an update: this is definitely non-trivial and not easy to solve, especially when considering deeply nested queries with bindvars. As this isn't currently a high priority issue, we've decided to shelve the idea for the time being.

@good-idea
Copy link

@Brian-McBride I needed something similar and came up with this.

@pluma, I know that the benefit of using the aql... tag is protection against AQL injection. Is this something that the tag takes care of, or, is it in the structure of db.query('MY AQL QUERY', { values: ... })?

To elaborate: My example creates a string that looks like

RETURN {
	"edges": (
		FOR u IN users LIMIT @after, @first
		FILTER u.locale == @locale
		RETURN {
			"cursor": u._id,
			"node": u
		}
	)
}

then calls

db.query(
 '...', // [the above query]
 {
   first: 10,
   locale: jp
 }
)

Is there anything unsafe about this?

@JonDum
Copy link

JonDum commented Jul 6, 2018

Thanks for sharing that, @good-idea. Pagination is turning out to be a huge bitch with arango, I really liked your example.

@pluma Just wanted to chime in that I share the same use case. Even just being able to dynamically add a statement would be of huge benefit, ie

let filter = args.evenOnly ? `FILTER i % 2 == true` : null
db.query(`FOR i IN 1..10 ${filter} RETURN i`)

Would it be possible to null check the arg at least?

aql`${null}`

would be treated safe

@pluma
Copy link
Contributor

pluma commented Sep 5, 2018

This has been implemented and will be part of the next release.

This means the following (adapted from @JonDum's example) will be possible:

let filter = args.eventOnly ? aql`FILTER i % 2 == ${args.reversed ? false : true}` : undefined;
db.query(aql`FOR i IN 1..10 ${filter} RETURN i`);

Note that this correctly merges the bindVars aql generates from the variables.

See the CHANGELOG for details.

@Nainterceptor
Copy link
Author

Thank you, @pluma ! 🎊

@JonDum
Copy link

JonDum commented Sep 5, 2018

That's awesome! You rock @pluma!

@alewaros
Copy link

alewaros commented Oct 5, 2018

Is there an ETA for the next release? And will the aql template present in transactions will also work like this?

@pluma
Copy link
Contributor

pluma commented Oct 22, 2018

@alewaros Transactions are executed on the server, so the ArangoDB version needs to support this as well. As far as I'm aware this feature isn't being backported to 3.3, so you need to use 3.4 or later. If you're using v3.4.0-rc2 or later, you can already give it a try. For production I recommend waiting for the regular release of 3.4.0.

I'm back from vacation and will work on dirty reads support this week. The current plan is for the next arangojs release to happen shortly before 3.4.0 GA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Request for new functionality to be added to the driver. semver-minor This issue does not require any backwards-incompatible changes to address.
Projects
None yet
Development

No branches or pull requests

7 participants