Use self._wrapper_cls in QueryBuilder.set

[jimorie]

This commit fixes a problem where the QueryBuilder.set method always
used the ValueWrapper class instead of the instance's _wrapper_cls.

Signed-off-by: Petter Nyström jimorie@gmail.com

[coveralls]

Coverage increased (+0.002%) to 97.724% when pulling 863a7ac on jimorie:use_wrapper_cls_in_updates into 89f70a7 on kayak:master.

[jimorie]

I have realized there are a lot of other places where the QueryBuilder's _wrapper_cls isn't used, so the fix in this PR is incomplete.

>>> t1 = pypika.Table("abc")
>>> t2 = pypika.Table("def")
>>> pypika.SQLLiteQuery.from_(t1).select(t1.id, True).where(t1.foo == True).join(t2).on(t2.bar == True)
SELECT "abc"."id",1 FROM "abc" JOIN "def" ON "def"."bar"=true WHERE "abc"."foo"=true
>>> pypika.SQLLiteQuery.into(t1).columns(t1.id, t1.foo).insert(1, True)
INSERT INTO "abc" ("id","foo") VALUES (1,true)

I kind of expected SQLiteQuery's custom ValueWrapper to turn all instances of True into 1 in the queries above. As it stands, it only seems to work in the select part.

Is this working as intended?

[twheys]

I looked through the code and this is the only instance where ValueWrapper is instantiated directly, rather than using _wrapper_cls aside from a couple of cases in the Query subclass for MySQL. Could fix the references there if you'd like but it's not presently posing a problems, so I don't think it's necessary to fix in the scope of this PR. Let me know if you'd like to fix it or not or if I should go ahead and merge this PR.

[jimorie]

I'm not really bothered about the use in the MySQL dialect code -- I assume it's not a problem there. But it seems to be a problem that the _wrapper_cls isn't used consistently in the core code.

I guess the problem is in Term.wrap_constant(). That method takes an optional _wrapper_cls argument, but that argument is only provided in one place (except dialects), and that is in the select code. If not given, it defaults to a ValueWrapper.

It looks like a lot of work to fix this, unfortunately. My first instict is that all Term objects need to be initialized with the wrapper class it should use by default. But there are a lot of places where new Terms are created...

Would you consider a change that introduced _wrapper_cls as an attribute on Term objects? Given all the added complexity and (possibly) overhead such a change would mean? I could perhaps look at it, but only if you think there is a possibility for it to be merged.

Do you have any other ideas how to make the SQLLite dialect's use of a custom ValueWrapper (and similar use cases) consistent?

[twheys]

Ah I see what you mean, definitely an oversight. I think the implementation of this _wrapper_cls function was just a quick fix for MSSQL for this case with booleans. Maybe a quicker and more effective solution would be to get rid of the SQLLiteValueWrapper class and move the boolean clause into the ValueWrapper.get_value_sql method. There are some other dialect specific cases handled in their respective classes, so it wouldn't deviate terribly, and given this is so far the only corner case with ValueWrapper, I think it can be viewed as a corner case and treated as such.

Something like this:

class ValueWrapper(Term):
    ...
    def get_value_sql(self, **kwargs):
        dialect = kwargs.get("dialect", None)
        ...
        if dialect == Dialects.MSSQL and isinstance(self.value, bool):
            return "1" if self.value else "0"
        if isinstance(self.value, bool):
            return str.lower(str(self.value))
        ...

Then we could just get rid of the _wrap_constant function entirely.

[jimorie]

Well, as a user of the library I definitely appreciate that I can write my own dialect and have it able to manipulate how it formats individual terms in the output, or attach side effects to them. I can see this being provided either by a configurable ValueWrapper as currently (although then it would have to be used consistently), or perhaps by adding some sort of optional converter parameter to the get_sql methods. It sounds as if this would not be as easy with what you're suggesting?

I can understand if you deem this as out of scope for what the library should offer, though.

For the record, my actual use case here was to write my own dialect that automatically turned all values used in the query into Parameter instances, and adding their values to a args dict on the query builder. So that I would get a parameterized sql query and have the arguments collected and ready to go. This looked pretty easy to do, had the wrapper_cls been used consistently. :)

[twheys]

I guess either way you would need to override the main Query class to make a dialect. There you could then override the _wrap_constant function, instead of providing a separate version of ValueWrapper for each dialect. Looks like the wrapper_cls might be inaccessible in a lot of places where it might be necessary to use, so it would certainly add complexity passing that around to all the different objects in the object tree

[jimorie]

Sorry, I forgot about this. Glad to see it merged even though it didn't do enough for my purposes of generating parameterized SQL. I ended up doing some post-processing in the QueryBuilder that replace all instances of ValueWrapper.value with instances of Parameter. Cheers!