Include the Engine when cloning a FromClause

[asherber]

Fixes #239

Turns out I was right the first time, even though my fork was out of date. If the Engine isn't preserved during FromClause.Clone(), then 2 of the 3 unit tests I added fail.

[asherber]

Before I launch into this long comment, I wanted to say how much I appreciate this library, and I'm sure there are considerations here that I'm missing.

Running some further tests has led me to other issues.

var query = new Query("Foo")
    .ForSqlServer(q => q.From("Bar"));

I would expect this to give me a table name of Bar for SqlServer and Foo for all other compilers. But when ClearComponent() gets called in the context of the second line, it deletes the first FromClause, because the clause's Engine is null. As a result, compiling this query fails for all other engines (because the only FromClause is for SqlServer).

public Q ClearComponent(string component, string engineCode = null)
{
    if (engineCode == null)
    {
        engineCode = EngineScope;
    }

    Clauses = Clauses
        .Where(x => !(x.Component == component && (engineCode == null || x.Engine == null || engineCode == x.Engine)))
        .ToList();

    return (Q)this;
}

I agree that if engineCode == null, the method should remove all clauses for that component. But if engineCode != null then I think it should only remove the clauses where x.Engine == engineCode. So I don't think that x.Engine == null belongs in the Where(). It looks like a couple of the Aggregate test cases fail if this change is made, because it breaks functionality in Compiler.TransformAggregateQuery(). That function does expect to clear all aggregate components – but it can accomplish that by calling query.ClearComponent("aggregate", null) instead of query.ClearComponent("aggregate", EngineCode).

It's a little complicated because I think that From() is the only method outside of a compiler that calls ClearComponent() and is therefore operating directly on the original query, not a clone, so what it does affects what all compilers will see.

There's a similar issue in BaseQuery.GetOneComponent(). This function first calls GetComponents(), which has the same kind of Where() as ClearComponent(). Here I think it's appropriate, however – you want to get the set of generic components as well as the ones that apply to the current engineCode. However, within GetOneComponent() I think that a component which matches engineCode should be prioritized over one where x.Engine == null. In my test case at the top of this comment, if I make the suggested change to ClearComponent() then all compilers work as expected except for SqlServer. When the SqlServer compiler calls GetOneComponent("from"), it gets back the generic FromClause because that happens to be first in the list, rather than getting back the FromClause which is specifically for SqlServer.

I get that this is complex because From() is a bit of an oddball. First, I think it's the only method outside of a compiler that calls ClearComponent(), so it's operating directly on the original query, not a clone, and therefore affects what all compilers will see. And second, each query can only have one FromClause per compiler.

Actually, each query can only have one Limit as well, and it looks like Limit() has some similar issues. In the following query, all compilers will get a limit of 10, because the second line overwrites the first:

var query = new Query("Foo")
    .Limit(5)
    .ForSqlServer(q => q.Limit(10));

That's a lot from what I thought was a small issue. I'm interested to hear what you think.

[asherber]

I'll add that both From() and Limit() can be made to behave better without changing ClearComponent() or GetOneComponent(), and I can put together a separate PR if you'd like to see what that looks like.

Edit: Hmm, no, I think I can work around ClearComponent(), but as long as GetOneComponent() doesn't prioritize an engine-specific component, I think there will be problems.

[ahmad-moussawi]

@asherber thanks for this great analysis, so I can conclude that we have the following issues here:

• GetComponent() should prioritize engine specific when passed.
• ClearComponent() should respect the engine context (This may need some discussions)

Is there something that I've missed?

[asherber]

While ClearComponent() doesn't work the way I would expect, I agree that it shouldn't be changed without further discussion. There may be side effects beyond what I've looked at.

Here's my current suggestion for GetOneComponent():

public C GetOneComponent<C>(string component, string engineCode = null) where C : AbstractClause
{
    engineCode = engineCode ?? EngineScope;

    var all = GetComponents<C>(component, engineCode);
    return all.FirstOrDefault(c => c.Engine == engineCode) ?? all.FirstOrDefault();
}

I think GetComponents() should continue to return components in native order from the list. But the code above tells GetOneComponent() to look first for a matching engine-specific component, and otherwise to take the first overall.

In addition, here's what I would suggest for From(). Something similar can be done for the other From() overloads, and for Limit(). The new AddOrReplaceComponent() refactors the approach currently used in Limit(), allowing us to maintain a single component for a given engine (or for a null engine).

public Q From(string table)
{
    var newClause = new FromClause
    {
        Table = table,
    };

    return AddOrReplaceComponent("from", newClause);
}

public Q AddOrReplaceComponent(string component, AbstractClause clause, string engineCode = null)
{
    engineCode = engineCode ?? EngineScope;

    var current = GetComponents(component).SingleOrDefault(c => c.Engine == engineCode);
    if (current != null)
        Clauses.Remove(current);

    return AddComponent(component, clause, engineCode);            
}

[asherber]

I can work all of this into this PR, if you agree.

[ahmad-moussawi]

@asherber you are right, agree from my side.

[asherber]

Okay, thanks. I'm a little swamped this weekend, will probably look at this early next week.

[asherber]

@ahmad-moussawi I think this is almost complete now. There's one thing which troubles me a little that has to do with Limit and Offset sharing a component.

var query = new Query("mytable")
    .Limit(5)
    .Offset(10)
    .ForMySql(q => q.Limit(20));

In the above example, I first set a generic limit and offset, and then I specify that MySql should have a different offset. As part of that code, the generic offset gets copied into the MySql LimitOffset. Now MySql has LIMIT 20 OFFSET 10 and all others have LIMIT 5 OFFSET 10. But now consider the following:

var query = new Query("mytable")
    .Limit(5)
    .Offset(10)
    .ForMySql(q => q.Limit(20))
    .Offset(50);

Here the generic offset of 10 gets copied into the MySql LimitOffset, but then the generic offset gets changed to 50. Looking just at the query, I would expect that MySql would use OFFSET 50, because that was the latest generic value given and there is no MySql value given. But since the MySql LimitOffset now has its own offset value (inherited from the first generic 10), it doesn't see the change to 50.

One could argue that this is a poorly formed query -- that the user shouldn't have provided two offsets. But I can see a case where a base query is set up, as in the first example, and then later in the code the generic offset is changed based on some other condition. Should the library handle this better? If so, then we'll need to stop copying the generic offset into the LimitOffset and instead have the compiler look further up the chain for the correct value.

(The generic LimitOffset would have limit 5, offset 50, and the MySql component would only have limit 20. When the MySql compiler looks at these values, it would see that there is no offset on that component and would then look at the generic component to see if there's an offset there.)

What do you think?

[asherber]

Hmm, not sure I can solve for this last case without one of these more drastic steps:

1. Include properties on LimitOffset to indicate whether the values should be inherited from the generic component.
2. Make Limit and Offset int?
3. Make Limit and Offset separate components/clauses

The problem is that a value of 0 could mean "look at the generic", or it could have been explicitly set.

[ahmad-moussawi]

@asherber You are totally right, this scenario can happen when using it dynamically, I think separating the limit and offset to two components is the best scenario here.

if you like, open another issue for it, so we can track it alone, and I will merge this PR for now.

[asherber]

Thanks, will do.

[asherber]

@ahmad-moussawi I've got the work on LimitOffset done. Since it builds on this PR, which hasn't been merged yet, okay if I just add the commit to this PR?

[ahmad-moussawi]

@asherber yes sure 😃

[ahmad-moussawi]

Merged with master now, Thanks!