Commits on Feb 15, 2017

1. Ensure aggregate functions enforce the column is from the right table …

   While working on #621, I noticed that these impls were incorrect and
   could be used to compile an incorrect query. I've corrected the impls
   and added the appropriate compile-fail test.
   
   I'm not sure if this was just an oversight or if I intentionally did
   this to avoid nullability somewhere. The latter is no longer relevant
   since we always make these expressions nullable now.

   

   sgrif committed Feb 15, 2017
   Configuration menu

   • View commit details
   • Copy full SHA for 200d13c
   • Browse repository at this point

   Copy the full SHA

   200d13c View commit details

   Browse the repository at this point in the history

2. Remove unused bounds and PhantomData from any and all …

   The phantom data was just plain unneccessary. It was either an oversight
   or a bug in older rust versions where `SqlType=Array<ST>` didn't count
   as the type being constrained. The `HasSqlType` constraints are
   sufficiently covered elsewhere (and frankly, I'm fairly certain that
   trait is useless and can be removed). It should be noted that we don't
   have a compile-test covering that case though, as pg is the only backend
   with additional types.

   

   sgrif committed Feb 15, 2017
   Configuration menu

   • View commit details
   • Copy full SHA for 76383ee
   • Browse repository at this point

   Copy the full SHA

   76383ee View commit details

   Browse the repository at this point in the history

3. Use associated types for SelectableExpression …

   The `SelectableExpression` trait serves two purposes for us. The first
   and most important role it fills is to ensure that columns from tables
   that aren't in the from clause cannot be used. The second way that we
   use it to make columns which are on the right side of a left outer join
   be nullable.
   
   There were two reasons that we used a type parameter instead of an
   associated type. The first was to make it so that `(Nullable<X>,
   Nullable<Y>)` could be treated as `Nullable<(X, Y)>`. We did this
   because the return type of `users.left_outer_join(posts)` should be
   `(User, Option<Post>)`, not `(User, Post)` where every field of `Post`
   is an `Option`.
   
   Since we now provide a `.nullable()` method in the core DSL, I think we
   can simply require calling that method explicitly if you want that tuple
   conversion to occur. I think that the most common time that conversion
   will even be used is when the default select clause is used, where we
   can just handle it for our users automatically.
   
   The other reason that we went with a type parameter originally was that
   it was easier, since we can provide a default value for a type parameter
   but not an associated type. This turned out to actually be a drawback,
   as it led to #104. This PR actually brings back aspects of that issue,
   which I'll get to in a moment.
   
   It's expected that any expression which implements
   `SelectableExpression<QS>` have a `T: SelectableExpression<QS>` bound
   for each of its parts. The problem is, the missing second parameter is
   defaulting to `T::SqlType`, which means we are implicitly saying that
   this bound only applies for `QS` which does not change the SQL type
   (anything except a left outer join). This ultimately led to #621.
   
   However, with our current structure, it is impossible to fix #621
   without re-introducing at least some aspects of #104. In
   #104 (comment) I
   said that we didn't need to worry about `1 + NULL`, because we didn't
   implement add for any nullable types. However, I'm not sure I considered
   joins when I made that statement. The statement applied to joins
   previously because of that implicit "sql type doesn't change"
   constraint. This commit removes that constraint, meaning #104 will be
   back at least when the nullability comes from being on the right side of
   a left join.
   
   I don't think this is a serious enough issue that we need to immediately
   address it, as the types of queries which would cause the issue still
   just don't happen in practice. We should come up with a long term plan
   for it, though. Ultimately the nullability of a field really only
   matters in the select clause. Since any operation on null returns null,
   and you basically want null to act as false in the where clasue, it
   doesn't matter there.
   
   So one partial step we could take is to break this out into two separate
   traits. One for the "make sure this is valid given the from clause", and
   one for the "make this nullable sometimes" case and only constrain on
   the first one in the where clause. We could then re-add the "sql type
   doesn't change" constraint on the problem cases, which will bring back
   aspects of #621, but only for select clauses which is a smaller problem.
   
   I'm not sure if I ultimately want to go the two traits route or not. If
   nothing else, the problem cases are much more obvious with this commit.
   Anywhere that has `type SqlTypeForSelect = Self::SqlType` is likely a
   problem case when joins are involved. This will make it easier to find
   all the places to apply a solution when I come up with one that I'm
   happy with.
   
   Fixes #621.

   

   sgrif committed Feb 15, 2017
   Configuration menu

   • View commit details
   • Copy full SHA for a4f49dd
   • Browse repository at this point

   Copy the full SHA

   a4f49dd View commit details

   Browse the repository at this point in the history