Adds support for Views.

[coderPaddyS]

This PR introduces the support of Views in a query-only version.
Views are implemented as a supertrait of Table without any primary key and no way to insert or update anything on them. To allow querying the bounds on select- and querydsl are lowered as required.
A view is created by using the diesel::view! based on the diesel::table! macro.
Diesel_cli successfully (at least on postgres more complicated views) the schema for views, but currently only as null-able fields. Further implementation is required to perhaps improve this situation.

This is somewhat related to #43 and only implements the easy-enough and is basically the implementation of #3473.
To improve the usability and usefulness of views one may add some features discussed there.

Remaining for this PR:

• Add tests to ensure correct behavior
• Add documentation
• Possibly rename Struct, Traits and functions
• Fix CI errors

[weiznich]

Thanks for opening this PR. I'm currently away for holidays. I will have a look at this PR as after I'm back.

[Ten0]

Hmm in some other places we name this around Source as the common denominator of view and table, it looks like this may be something we want to be consistent with here.

[coderPaddyS]

Thanks for opening this PR. I'm currently away for holidays. I will have a look at this PR as after I'm back.

Have a nice and well deserved holiday ^^

[kw217]

Hi @coderPaddyS - thanks for making a great start at this! It will be great to have view support. This PR is a bit more complex than it needs to be, and there's a bit more work to do until it's complete - I'll try to outline what's needed.

I agree with the #3473 approach of doing the simplest part first, i.e., read-only views with all fields nullable. So please can you keep this PR entirely about that, with no discussion or code that serves the future rather than this goal. For example:

• The comment at the top of this PR starts a long discussion about nullability, updating, and automatic joins. If anyone takes you up on this discussion, all it can do is delay this PR getting merged. That discussion belongs on Should we have a story for database views? #43 or a future discussion thread, not here - please can you drop it from here, and start it elsewhere. Then we can get this PR merged and gain experience that will help move that discussion forward.
• The code changes are more invasive than necessary. I don't think you need all_columns until you need to support insert/delete/update/append; if you don't need all_columns then you don't need to be able to generate Columns for a view; and so you don't need to relax Column::Table. Since this PR only addresses the read-only case, none of this is necessary. You also probably don't need a separate View trait (or at least, it's very simple with no methods and there's no need for it to be a supertrait of Table). And avoiding type changes means you don't need to consider whether the changes make error messages worse.

I suspect with these changes you can make the PR a lot simpler, which will also make it easier to test and review. Please also ensure CI is passing, ensuring that all clippy and rustfmt issues have been addressed before review.

Thanks again - and do let me know if anything above is unclear or if I've misunderstood something.

[Ten0]

The comment at the top of this PR starts a long discussion about nullability

As far as I'm concerned I was happy to read the note about already already-done investigation and potential future evolutions on this topic, as that is something I'd likely have felt the need for investigating as well otherwise, upon noticing that everything was marked nullable 😄
That being said the rationale about why everything is nullable and why it's hard to do otherwise could probably live as a comment in the code, next to where we actually make things nullable.

[kw217]

As far as I'm concerned I was happy to read the note about already already-done investigation and potential future evolutions on this topic, as that is something I'd likely have felt the need for investigating as well otherwise, upon noticing that everything was marked nullable 😄 That being said the rationale about why everything is nullable and why it's hard to do otherwise could probably live as a comment in the code, next to where we actually make things nullable.

Thanks! Yes I agree it is necessary to have a comment here explaining why everything is nullable! But IMHO it would be best to keep discussion about how to fix that later in a separate thread.

[coderPaddyS]

• The comment at the top of this PR starts a long discussion about nullability, updating, and automatic joins. If anyone takes you up on this discussion, all it can do is delay this PR getting merged. That discussion belongs on Should we have a story for database views? #43 or a future discussion thread, not here - please can you drop it from here, and start it elsewhere. Then we can get this PR merged and gain experience that will help move that discussion forward.

Gracefully done :D

• The code changes are more invasive than necessary. I don't think you need all_columns until you need to support insert/delete/update/append; if you don't need all_columns then you don't need to be able to generate Columns for a view; and so you don't need to relax Column::Table. Since this PR only addresses the read-only case, none of this is necessary. You also probably don't need a separate View trait (or at least, it's very simple with no methods and there's no need for it to be a supertrait of Table). And avoiding type changes means you don't need to consider whether the changes make error messages worse.

So If I get you right, you suggest removing the View trait and instead using QuerySource + Sized or a similar traitbound? The issue I got in my first try using QuerySource instead of using Table is in this impl of AppearsInFromClause where rustc informs me of the following issue, referencing this other AppearsInFromClause impl:

conflicting implementations of trait `query_source::AppearsInFromClause<_>` for type `select_statement::SelectStatement<_>`
downstream crates may implement trait `query_source::QuerySourceNotEqual<_>` for type `query_builder::select_statement::SelectStatement<_>`

I currently don't see how I fix that without introducing another trait or changing seemingly unrelated code.

I suspect with these changes you can make the PR a lot simpler, which will also make it easier to test and review. Please also ensure CI is passing, ensuring that all clippy and rustfmt issues have been addressed before review.

I'm not that well versed with clippy and multiple crate features. The only issue clippy reports is using only the sqlite-feature in the diesel_cli having some unused methods due to them being used with the mysql- or postgres feature. Is it common practice to check for multiple features on such impl blocks?

[kw217]

Thanks :-)

On the clippy problem, if you look in https://github.com/diesel-rs/diesel/blob/master/diesel_cli/Cargo.toml#L74C1-L82C29 you see that there's an internal feature uses_information_schema, and this is used in https://github.com/diesel-rs/diesel/blob/master/diesel_cli/src/infer_schema_internals/mod.rs#L6C1-L7C24 to control whether the module is included at all. I think you just need to use the same feature to control whether your new code (e.g., SupportedColumnStructures) is included.

On the bigger issue, it's obviously more complex than I hoped. I'm working to get my head around how it currently works (I've not delved into this before) - it may be that changing existing code as you have done is unavoidable. We may have to wait until @weiznich returns to confirm.

[kw217]

Ah, this is interesting. I was wrong earlier - we do still need view! to define the columns of the view, and each column needs a Table so that we can select it. And there's no harm in defining AllColumns. Instead I think we should prevent insert/update/delete by having the view simply not implement the relevant traits, so that you simply cannot call insert_into, update, delete, copy_to, and friends.

We can get part of the way there by omitting the IntoUpdateTarget impl from the generated code. That prevents update and delete. But sadly insert_into and friends just take Table, not some IntoInsertTarget trait, so that's not so easy. (There's no point omitting Insertable; that's just one way of inserting, not the only way.)

This suggests that we could add a new marker trait IntoInsertTarget : Table and have table! generate an impl for it but view! not. And then we could modify insert_into and friends to require it.

I think this would achieve the desired effect, while changing a lot less code.

• Notice that this keeps the name Table to refer to both tables and views. I think that's the least confusing approach overall (and I'd justify it by saying views are table-ish), but if we didn't like it we could simply rename throughout; it doesn't affect the semantics.
• Would this be a breaking change? I presume we'd rather not break existing code. However code that generically works with Table and assumes it can insert/delete it is presumably rare so maybe not a concern?

What do you think?

[Ten0]

IntoInsertTarget : Table

It's unclear to me why a new IntoInsertTarget is useful here if it's only implemented by Table.
In addition I think we already have a such trait: AsChangeset.
At least I would be very surprised if making Table a supertrait of that trait was the ideal approach: it seems that this would close the API of implementing it on anything that's not Table.

[kw217]

It's unclear to me why a new IntoInsertTarget is useful here if it's only implemented by Table.

It's not implemented by Table, it's explicitly implemented by each table that is autogenerated by table!, just like IntoUpdateTarget. That way it can be omitted by view!.

[Ten0]

SQL-wise, a table is not a view, so I think we need a different naming for the concept of "either a view or a table", because the code is otherwise going to be very hard to read.
(This is linked to #4077 (comment))

(It's clear from just this line and the documentation of View that there's something wrong: View documentation says it represents an SQL database view generated by view!, but Table: View and types that implement Table don't match that)

[kw217]

Yes, I also agree there's something wrong here.

[czotomo]

(at least in Postgres) Views, Materialized Views and Tables are all "Relations" so maybe that's a good starting point when it comes to naming?

[Ten0]

It mentions that "composite types and indexes are relations" as well but that may still be a good naming, if we need one that's separate from QuerySource.

[weiznich]

I agree with @Ten0 here that View shouldn't be the super trait of Table. Instead I would propose the following structure:

trait Relation: QuerySource + AsQuery + Sized {
    type AllColumns: SelectableExpression<Self> + ValidGrouping<()>;

    /// Returns a tuple of all columns belonging to this table.
    fn all_columns() -> Self::AllColumns;
}

trait Table: Relation {
    /// The type returned by `primary_key`
    type PrimaryKey: SelectableExpression<Self> + ValidGrouping<()>;

    /// Returns the primary key of this table.
    ///
    ///
    /// If the table has a composite primary key, this will be a tuple.
    /// If the table has a composite primary key, this will be a tuple.
    fn primary_key(&self) -> Self::PrimaryKey;
}

trait View: Relation {
 // for now only a marker trait?
}

I think the "Relation" name suggested by @czotomo is a good choice here for the new trait with the shared behavior.

By having the super trait relation we also ensure that this isn't a breaking change and by having a seperate View trait that is not implemented for Table (and Table is not implemented for View by default) we can use that to differentiate behavior between these two. Also I think this opens up the road to add more table like constructions, like sql functions returning a "table`, in the future more easily.

For the time being I wouldn't want to expose the structures purely based on the Relation trait (without implementing Table nor View) in an easy way for the user.

[weiznich]

I had another look at this and unfortunately the suggested trait structure won't be possible without breaking changes. In fact we cannot remove items from the Table trait without causing locations that manually implement this trait to break.

I think we might be able to workaround this issue by using the following structure:

trait Relation: QuerySource + AsQuery + Sized {
    type AllColumns: SelectableExpression<Self> + ValidGrouping<()>;

    /// Returns a tuple of all columns belonging to this table.
    fn all_columns() -> Self::AllColumns;
}

trait Table: QuerySource {
    // unchanged
}

impl<T> Relation for T where T: Table {
    // forward relevant types/functions to the new trait
}

trait View: Relation {
 // for now only a marker trait?
}

And then change most of the existing AsQuery impls to use the new Relation trait instead. By that we won't break any existing usage, but can use the new trait in the relevant places. We might want to have a look at the impact on the compiler error messages there.

[Ten0]

It's not implemented by Table, it's explicitly implemented by each table that is autogenerated by table!, just like IntoUpdateTarget. That way it can be omitted by view!.

Given the supertrait constraint that I'm commenting on:

IntoInsertTarget : Table

it seems impossible for anything but a T: Table to implement the suggested IntoInsertTarget trait.
My comment is relative not to how the concrete implementation is generated, but to the constraint itself.

[kw217]

it seems impossible for anything but a T: Table to implement the suggested IntoInsertTarget trait.

I think it could probably be relaxed to QuerySource, but today insert_into is only defined on Table so I'm preserving that. It's sufficient, since in my proposal each view implements Table (I argued for this above, and noted the alternative that we could rename Table to Tableish throughout Diesel. I think it's simpler to just keep Table.)

[coderPaddyS]

new marker trait IntoInsertTarget : Table and have table! generate an impl for it but view!

I like the gist of that idea. As you've mentioned, this would prevent the modification of views entirely for the moment, but lets the API just open enough to add backend-specific support for modification later on in the view! macro expansions.
As far as I see, IntoInsertTarget then needs to provide the relevant information for the primary keys, but isn't that more or less just a swap of View and Table (without the overhead of renaming everything to match View at the end and just refactoring all modification APIs)?

[kw217]

Cool! Yes, but I think not renaming everything is a win - there's a lot of tricky code that checks for appearances of Table etc, which it would be much simpler not to disturb. And I don't think you even need to define a View trait at all - it won't be used for anything. As @Ten0 has pointed out, tables and views are not subtypes of each other either way round; arguably we could introduce a new supertype for "tableish" things but it seems unnecessary.

[coderPaddyS]

Then what's your opinion on separating Table and Table::PrimaryKey by adding another trait HasPrimaryKey?
In my above suggestion, Table::PrimaryKey is already extracted into InsertIntoTarget::PrimaryKey, but this does not feel right. The FindDsl should be used by everything that has a primary key specified, however my suggestion above would implement the FilterDsl only for anything that can be inserted into.
The Downside of either approach suggested would enable users to manually implement Tables without specifying a primary key as it is disconnected from Table. That's something not wanted as stated by @weiznich #3473 a year ago.

I'm fine with implementing just InsertIntoTarget with InsertIntoTarget::PrimaryKey and moving this discussion to elsewhere as the question if this is wanted is not relevant for the implementation of views.

[kw217]

Ah, I'd missed that comment from @weiznich. It does say pretty clearly not to use Table for views.

But there's a tension here: as we've seen, views do need to be treated like tables, in that they appear as field qualifiers (view.field) and so need to be checked for appearance/uniqueness in queries (AppearsIn...). Yet they're also not like tables in that they don't have a primary key.

We have to pick one - either views are Tables that don't have primary keys, and so tables are field qualifiers but don't always have primary keys, or field qualifiers become something else like Tableish, views and tables are distinct, and tables always have a primary key.

I think the first is probably less disruptive, but I'm not certain, and it sounds like @weiznich might have the opposite preference. Do you have a sense of the impact and pros/cons of each approach?

[coderPaddyS]

My two cents on this decision, but firstly a small point on my normal contact with views:
I'm not a database engineer or anything comparable, but for my small(ish) projects I do use many tables and I like to encapsulate all relevant logic inside the database. This often requires more complex queries run somewhat often at different places. In code, I'd extract that query into a function, in a database into a view rather than a stored procedure. But most views mostly operate on one set of tables, e.g. adjoining data by foreign keys. In this sense, I expect views to just behave like tables, but just logically. I usually don't modify entries of a view.
In that sense, I think of a view as a subset of operations on a table, bringing up my initial model of View being a supertrait of Table.

While it is possible to have a table without primary key, I do share the opinion of @weiznich to ensure a primary key is used.
However, I'd also be fine with a solution (1) where it is possible to create such a table, but only manually. diesel_cli can easily just support tables with primary key. For this Table would just require AllColumns and in the table! expansion we'd generate impls for the primary key and table modification operations. If a user requires to have a table without primary key, they could implement the relevant traits for their table manually or use a patch-file to mark that table as a view.
Another option (2) would be to add a supertrait Tableish of Table as mentioned requiring just the specification of the columns whilst Table would require a PrimaryKey as currently. A view is just a Tableish object. Implementation for modification operation would happen as auto impls for Table or manually for something implementing Tableish.

Regarding the implementation overhead, I expect the latter one to require more altercations, especially the whole query related API, due to the supertrait. So I sense modifications similar to the initial version of this PR. Both implementations from above require the introduction of several traits ((only latter) supertrait, primary key trait and perhaps other modification traits) and thus renaming and altering bounds allover the place.
In comparison to your idea @kw217 regarding views to be Tables, this would be comparable to (1).

[weiznich]

First I would like to thank @coderPaddyS for working on this. This is already a really good starting point ❤️

I would also thank @Ten0, @kw217 and @czotomo for their helpful reviews and comments. That made it much easier for me to come back from holiday and dive into a complex PR like that one.

I left some comments about the naming of the fundamental traits and a structure that I would consider fitting.

On a more fundamental level: I think it's fine to start with something rather barebone for now, which means marking all view columns as Nullable + not trying to infer any relations at all. I would even consider to put this, for now, behind an explicit, experimental opt-in flag in diesel-cli so that we can improve the generation code in later version. (For example adding a way to infer whether a column is not null, etc)

Finally: I've not done a full review of all changes in this PR. Before doing that I would like to see at least some tests in diesel_tests + in diesel_cli (for the print-schema part). Maybe we should even consider splitting the PR into two parts. The first part could add the technical foundation in diesel + tests in diesel_tests, while the second part would add support for generating view! calls to diesel_cli? That would keep the changeset smaller and easier to review.

[weiznich]

I agree with @Ten0 here that View shouldn't be the super trait of Table. Instead I would propose the following structure:

trait Relation: QuerySource + AsQuery + Sized {
    type AllColumns: SelectableExpression<Self> + ValidGrouping<()>;

    /// Returns a tuple of all columns belonging to this table.
    fn all_columns() -> Self::AllColumns;
}

trait Table: Relation {
    /// The type returned by `primary_key`
    type PrimaryKey: SelectableExpression<Self> + ValidGrouping<()>;

    /// Returns the primary key of this table.
    ///
    ///
    /// If the table has a composite primary key, this will be a tuple.
    /// If the table has a composite primary key, this will be a tuple.
    fn primary_key(&self) -> Self::PrimaryKey;
}

trait View: Relation {
 // for now only a marker trait?
}

I think the "Relation" name suggested by @czotomo is a good choice here for the new trait with the shared behavior.

By having the super trait relation we also ensure that this isn't a breaking change and by having a seperate View trait that is not implemented for Table (and Table is not implemented for View by default) we can use that to differentiate behavior between these two. Also I think this opens up the road to add more table like constructions, like sql functions returning a "table`, in the future more easily.

For the time being I wouldn't want to expose the structures purely based on the Relation trait (without implementing Table nor View) in an easy way for the user.

[weiznich]

That should follow the Relation based naming then as well.

[weiznich]

While I can understand the motivation for renaming this field, it remains a breaking change. I would rather go with

Suggested change

	type Source: QuerySource;
	type Table: Releation;

instead. (As Relation is the super trait of Table that's fine)

Maybe we can also add a Fixme(diesel 3.0): rename to Source or something like that.

[weiznich]

Minor stylistic nit: Diesel we use unnested imports in all other parts of the code base, so I would like to keep this consistent.