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

fix: Including aliasing in scoping strategey, too. #839

Merged
merged 2 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@
@API(status = INTERNAL, since = "2021.1.0")
abstract class LiteralBase<T> implements Literal<T> {

/**
* A literal for the blank.
*/
static final Literal<String> BLANK = new LiteralBase<>(" ") {
@Override
public String asString() {
return content;
}
};

/**
* The content of this literal.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,24 +38,14 @@
@API(status = STABLE, since = "1.0")
public final class Merge extends AbstractClause implements UpdatingClause {

/**
* A literal for the blank.
*/
static final Literal<String> BLANK = new LiteralBase<>(" ") {
@Override
public String asString() {
return content;
}
};

private final Pattern pattern;
private final List<Visitable> onCreateOrMatchEvents;

Merge(Pattern pattern, List<MergeAction> mergeActions) {
this.pattern = pattern;

this.onCreateOrMatchEvents = new ArrayList<>();
this.onCreateOrMatchEvents.add(BLANK);
this.onCreateOrMatchEvents.add(LiteralBase.BLANK);
this.onCreateOrMatchEvents.addAll(mergeActions);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -494,7 +494,7 @@ void enter(org.neo4j.cypherdsl.core.Condition condition) {
}

void enter(Literal<?> literal) {
if (literal instanceof Asterisk || literal instanceof PeriodLiteral || literal instanceof RawLiteral.RawElement || literal == Merge.BLANK || literal == ListOperator.DOTS || literal instanceof Namespace) {
if (literal instanceof Asterisk || literal instanceof PeriodLiteral || literal instanceof RawLiteral.RawElement || literal == LiteralBase.BLANK || literal == ListOperator.DOTS || literal instanceof Namespace) {
return;
}
this.literals.add(literal);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import org.neo4j.cypherdsl.core.Expression;
import org.neo4j.cypherdsl.core.Foreach;
import org.neo4j.cypherdsl.core.IdentifiableElement;
import org.neo4j.cypherdsl.core.MapProjection;
import org.neo4j.cypherdsl.core.Named;
import org.neo4j.cypherdsl.core.Order;
import org.neo4j.cypherdsl.core.PatternComprehension;
Expand Down Expand Up @@ -115,6 +116,13 @@ public static ScopingStrategy create(List<BiConsumer<Visitable, Collection<Ident
private final List<BiConsumer<Visitable, Collection<IdentifiableElement>>> onScopeEntered = new ArrayList<>();
private final List<BiConsumer<Visitable, Collection<IdentifiableElement>>> onScopeLeft = new ArrayList<>();

/**
* A flag if we can skip aliasing. This is currently the case in exactly one scenario: A aliased expression passed
* to a map project. In that case, the alias is already defined by the key to use in the projected map, and we
* cannot define him in `AS xxx` fragment.
*/
private final Deque<Boolean> skipAliasing = new ArrayDeque<>();

private ScopingStrategy() {
this.dequeOfVisitedNamed.push(new LinkedHashSet<>());
}
Expand Down Expand Up @@ -150,6 +158,12 @@ public void doEnter(Visitable visitable) {
this.currentImports.compareAndSet(null, imports);
}

if (visitable instanceof MapProjection) {
this.skipAliasing.push(true);
} else if (visitable instanceof SubqueryExpression) {
this.skipAliasing.push(false);
}

boolean notify = false;
Set<IdentifiableElement> scopeSeed = dequeOfVisitedNamed.isEmpty() ? Collections.emptySet() : dequeOfVisitedNamed.peek();
if (hasLocalScope(visitable)) {
Expand All @@ -167,6 +181,10 @@ public void doEnter(Visitable visitable) {
}
}

public boolean isSkipAliasing() {
return Optional.ofNullable(this.skipAliasing.peek()).orElse(false);
}

/**
* @param namedItem An item that might have been visited in the current scope
* @return {@literal true} if the named item has been visited in the current scope before
Expand Down Expand Up @@ -226,6 +244,10 @@ public void doLeave(Visitable visitable) {
this.definedInSubquery.pop();
}

if (visitable instanceof MapProjection || visitable instanceof SubqueryExpression) {
this.skipAliasing.pop();
}

if (hasImplicitScope(visitable)) {
notify = true;
this.implicitScope.pop();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
import org.neo4j.cypherdsl.core.ListExpression;
import org.neo4j.cypherdsl.core.Literal;
import org.neo4j.cypherdsl.core.MapExpression;
import org.neo4j.cypherdsl.core.MapProjection;
import org.neo4j.cypherdsl.core.Match;
import org.neo4j.cypherdsl.core.Merge;
import org.neo4j.cypherdsl.core.MergeAction;
Expand Down Expand Up @@ -149,13 +148,6 @@ class DefaultVisitor extends ReflectiveVisitor implements RenderingVisitor {
*/
private final Deque<AliasedExpression> currentAliasedElements = new ArrayDeque<>();

/**
* A flag if we can skip aliasing. This is currently the case in exactly one scenario: A aliased expression passed
* to a map project. In that case, the alias is already defined by the key to use in the projected map and we
* cannot define him in `AS xxx` fragment.
*/
private boolean skipAliasing = false;

/**
* A cache of delegates, avoiding unnecessary object creation.
*/
Expand Down Expand Up @@ -259,10 +251,6 @@ protected boolean preEnter(Visitable visitable) {
currentAliasedElements.push(aliasedExpression);
}

if (visitable instanceof MapProjection) {
this.skipAliasing = true;
}

int nextLevel = ++currentLevel + 1;
if (visitable instanceof TypedSubtree) {
enableSeparator(nextLevel, true);
Expand Down Expand Up @@ -325,10 +313,6 @@ protected void postLeave(Visitable visitable) {
currentAliasedElements.pop();
}

if (visitable instanceof MapProjection) {
this.skipAliasing = false;
}

if (visitable instanceof AliasedExpression aliasedExpression) {
visitableToAliased.add(aliasedExpression);
}
Expand Down Expand Up @@ -445,7 +429,7 @@ void enter(AliasedExpression aliased) {

void leave(AliasedExpression aliased) {

if (!(this.visitableToAliased.contains(aliased) || skipAliasing)) {
if (!(this.visitableToAliased.contains(aliased) || scopingStrategy.isSkipAliasing())) {
builder.append(" AS ").append(escapeIfNecessary(nameResolvingStrategy.resolve(aliased, true, inLastReturn())));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1788,4 +1788,81 @@ void sequentialExistingSubqueriesShouldNotHaveScopingIssues() {
+ "}) "
+ "RETURN true");
}

@Test // GH-838
void aliasMustBeIncludedInSubqueries1() {
var n1 = Cypher.node("Foo").named("n1");
var point = Cypher.name("point");
var resultStatement = Cypher.match(n1)
.returning(n1.project("points", Expressions.collect(
Cypher.unwind(n1.property("points")).as(point)
.returning(Cypher.listOf(point.property("x"), point.property("y")))
.build()
)))
.build();

assertThat(resultStatement.getCypher())
.isEqualTo("MATCH (n1:`Foo`) "
+ "RETURN n1{"
+ "points: COLLECT {"
+ " UNWIND n1.points AS point"
+ " RETURN [point.x, point.y]"
+ " }"
+ "}");
}

@Test // GH-838
void aliasMustBeIncludedInSubqueries2() {
var n1 = Cypher.node("Foo").named("n1");
var point = Cypher.name("point");
var points = Cypher.name("points");
var resultStatement = Cypher.match(n1)
.returning(n1.project("points", org.neo4j.cypherdsl.core.Expressions.collect(
Cypher
.with(n1.property("points").as(points))
.unwind(points).as(point)
.returning(Cypher.listOf(point.property("x"), point.property("y")))
.build()
)))
.build();

assertThat(resultStatement.getCypher())
.isEqualTo("MATCH (n1:`Foo`) "
+ "RETURN n1{"
+ "points: COLLECT {"
+ " WITH n1.points AS points"
+ " UNWIND points AS point"
+ " RETURN [point.x, point.y]"
+ " }"
+ "}");
}

@Test // GH-838
void aliasMustBeIncludedInSubqueries3() {
var n1 = Cypher.node("Foo").named("n1");
var n2 = Cypher.node("Bar").named("n2");
var point = Cypher.name("point");
var resultStatement = Cypher.match(n1)
.returning(n1.project("points", Expressions.collect(
Cypher.unwind(n1.property("points")).as(point)
.match(n2)
.where(n2.property("loc").eq(point))
.returning(
n2.project(n2.property("y").as("x"), Expressions.collect(Cypher.match(Cypher.node("FooBar").named("fb")).returning(Cypher.name("fb").as("foo")).build()).as("y"))
)
.build()
)))
.build();

assertThat(resultStatement.getCypher())
.isEqualTo("MATCH (n1:`Foo`) "
+ "RETURN n1{"
+ "points: COLLECT {"
+ " UNWIND n1.points AS point"
+ " MATCH (n2:`Bar`)"
+ " WHERE n2.loc = point"
+ " RETURN n2{x: n2.y, y: COLLECT { MATCH (fb:`FooBar`) RETURN fb AS foo }}"
+ " }"
+ "}");
}
}