splitting into multiple schema.rs #3796

forest1102 · 2023-09-17T13:40:29Z

fixes #3799
I made an additional property sub_print_schema in diesel.toml to output multiple schema.rs.
Tables included in the output code of sub_print_schema is first filtered by print_schma.

Signed-off-by: forest1102 <[email protected]>

weiznich · 2023-09-18T10:48:32Z

Thanks for opening this PR.

As written in the linked issue, we are generally interested in a solution to this problem, although I'm not sure if I personally would like to include a new sub_print_schema field for that opposed to just allowing several occurrences of [print-schema]. Can you explain what's the reasoning why we should want to use the proposed implementation instead?

In addition to the fundamental technical question, this PR would also need to have some tests if we decide to go this way.

forest1102 · 2023-09-19T01:23:49Z

@weiznich
There are two main reasons why I choose to add sub_print_schema

Simplicity of coding
When making the type of print_schema one or more print_schema, there are lots of stuff to change. For instance, implementing a diff-schema will be tough. Instead of doing that, I decided to make diff_schema have only interest in the main print_schema field.
Consistency of CLI parameters
When running diesel migration run with specified targets or other specifiers as a parameter, if allowing multiple print_schema, I wonder how those parameters are applied. Regarding sub_print_schema, specifiers of tables interested by CLI will be used to print_schema and print_sub_schema will filter the tables specified by print_schema. In that way, specified parameters of CLI and diesel.toml are not wasted.

If this fits your thoughts too, I will add some tests.

Sytten · 2023-09-19T13:09:51Z

Just adding my 2cents since I also wanted to add SQLite attached table supports at some point.

I think it would be better to have multiple print schéma occurrences so we can print each schema / attached table in a different file. Though the print sub schema could be used to print them all in the same file.

I am actually wondering if we could remove the error when schema is set for sqlite in the CLI with this PR.

forest1102 · 2023-09-20T01:00:14Z

@Sytten
Sorry, I don't understand what you are telling.
Print schema will be used for aggregating all tables used.
Sub print schema will be used for splitting table schema, and parallelizing compiling.
What do you mean by the error when using sqlite?

Sytten · 2023-09-20T01:06:39Z

Unsure what your PR does, but if it allows printing multiple database schemas like the issue mentioned, then it should be possible to remove the following limitation in the sqlite inference:

diesel/diesel_cli/src/infer_schema_internals/sqlite.rs

Line 39 in d4a8a67

if schema_name.is_some() {

Otherwise it doesn't fix the issue as mentioned and we should remove the fixes comment.

forest1102 · 2023-09-20T02:53:45Z

@Sytten
I still don't understand your question.
This PR is to print out multiple schema.rs files using sub_print_schema, and sqlite cannot make multiple schema names.

Sytten · 2023-09-20T03:25:03Z

In your PR description is says fixes #1728, this issue is to support multiple database schema in one schema.rs file so I don't think this closes the issue. In Sqlite you can attach databases which act similar to multiple schemas in other databases. If your PR was to support multiple database schema (which again is the issue #1728), then we could likely remove the check I linked above. Since this PR doesn't address this issue, you should remove the fixes #1728 and ignore my comments :)

forest1102 · 2023-09-20T09:44:11Z

@Sytten
Thanks for the suggestion. I created issue and changed addressed issue.

weiznich · 2023-09-20T11:13:40Z

There are two main reasons why I choose to add sub_print_schema

Simplicity of coding
When making the type of print_schema one or more print_schema, there are lots of stuff to change. For instance, implementing a diff-schema will be tough. Instead of doing that, I decided to make diff_schema have only interest in the main print_schema field.

Consistency of CLI parameters
When running diesel migration run with specified targets or other specifiers as a parameter, if allowing multiple print_schema, I wonder how those parameters are applied. Regarding sub_print_schema, specifiers of tables interested by CLI will be used to print_schema and print_sub_schema will filter the tables specified by print_schema. In that way, specified parameters of CLI and diesel.toml are not wasted.

Thanks for writing up your reasoning. I do not really agree on these conclusions. I can follow your argumentation in so far that it's easier to implement it as new feature instead of extending an existing old feature and provide support for all the functionality the existing feature has, but for me that only sounds like getting around of doing the work for implementing the proper way. As for the CLI flags: Those could appear more than once.

To summarize that: I would prefer having an implementation that uses several real print-schema instances instead of introducing a new striped down feature for this.

forest1102 · 2023-09-20T14:37:54Z

@weiznich
Another benefit of sub_print_schema is that searching tables and foreign keys runs for generating print_schema and sub_print_schema are generated by reusing those searched tables foreign keys.
Also, I think it is hard to pass multiple print_schema parameters in CLI. For instance, if I want to update 1st and 3rd print_schema field using CLI parameters, it cannot be achieved by current CLI passing program.

weiznich · 2023-09-22T19:04:49Z

I'm sorry, but this still reads like an unnecessary duplication of features for me. I'm really not that sure that it is the right behavior ro share foreign key definitions between the different schemas.

As for the CLI: Yes, that would likely require new CLI parameters to control the new functionality.

forest1102 · 2023-10-05T06:56:16Z

@weiznich
Thanks for the suggestions.
Now, I rethought implementations.
Can you give an opinion on my following specifications to make this feature?

The New diesel.toml format allows a hashmap of a print_schema field with the name of the schema.

for print_schema command
- when print_schema field is just a single print_schema
  - No changes
- when it is a hashmap of print_schema
  - when no options for table or name specified
    - run each of print_schema in diesel.toml
  - when multiple name parameters and partia print_schema parameter are specified
    - matched name of print_schema will be modified
      e.g. diesel migration run --name user_schema --only-tables user --name company_schema --only-tables company will modify user_schema only_tables field and company company_schema field.
    - no matching print_schema name will return an error.
  - no name is specified and print_schema parameters are specified
    - all diesel.toml field is dismissed, and run with the specified parameter.
for diff_schema command
when print_schema field is just a single print_schema
- No changes
when it is a hashmap of print_schema
- with name parameter
  - run with matched name
- without name parameter
  - return error

weiznich · 2023-10-06T12:21:44Z

I'm sorry but I cannot really follow what you are suggesting. Can you maybe try to provide some examples of the old and new (envisioned) diesel.toml format and corresponding CLI calls for these examples?

forest1102 · 2023-10-06T12:47:38Z

@weiznich
Suppose following diesel.toml

[print_schema.user]
file="user/src/schema.rs"
[print_schema.company]
file="company/src/schema.rs"
filter={only_tables=["companies"]}

diesel migration run --name user --only-tables users will override user's filter and run command.

diff_schema requires to set name parameter if print_schema is a hashmap, and the specified name of print_schema will be applied

weiznich · 2023-10-09T13:45:58Z

@forest1102 Thanks for spelling out that proposal. I think I would be fine with such a change, with some minor additions:

I wouldn't want to use --name for it. I think something more specific like --print-schema-key or similar would fit better.
It should also allow to set the actual database schema on each instance. So something like:)

[print-schema.different-thing]
schema = "bar"
file = "src/schema_bar.rs"

should be possible

What about applying the corresponding CLI action to all defined schemas if no specific --print-schema-key flag is given?
( Likely we should disallow [print-schema.default] and keep that one for the existing [print-schema] entry?

forest1102 · 2023-10-11T07:27:28Z

@weiznich
Thanks for the suggestion.
I agree with your idea that --print-schema-key is more suitable.
But, I didn't understand applying the corresponding CLI action when not specifying --print-schema-key flag, so can you give me some examples? And what is the meaning of disallowing [print-schema.default]?

should be possible

* What about applying the corresponding CLI action to **all** defined schemas if no specific  `--print-schema-key` flag is given?

* ( Likely we should disallow `[print-schema.default]` and keep that one for the existing `[print-schema]` entry?

weiznich · 2023-10-11T09:10:34Z

But, I didn't understand applying the corresponding CLI action when not specifying --print-schema-key flag, so can you give me some examples?

Let's assume the following config:

[print_schema]
file = "src/schema.rs"
filter={only_tables=["bar"]}

[print_schema.user]
file="user/src/schema.rs"
filter={only_tables=["users"]}

Then diesel print-schema --print-schema-key user should only perform the print-schema command defined for in the print_schema.user entry, so it should only change user/src/schema.rs. diesel print-schema (without any key) should act like you are executing diesel print-schema --print-schema-key key for all defined keys. So it should change both schema files. diesel print-schema --print-schema-key default should then refer to the [print-schema] entry itself, so it should only change the src/schema.rs file.

forest1102 · 2023-10-12T04:04:18Z

@weiznich
Thanks for explanation and all of your suggestions. I will implement those features, however I don't know how to parse hashmap with empty key like you wrote as a example.

[print_schema]
file = "src/schema.rs"
filter={only_tables=["bar"]}

[print_schema.user]
file="user/src/schema.rs"
filter={only_tables=["users"]}

weiznich · 2023-10-12T18:31:33Z

I would probably use an explicit insert into the HashMap after the parsing step using default as key.

Signed-off-by: forest1102 <[email protected]>

forest1102 · 2024-02-02T05:39:34Z

@weiznich
Are my tests sufficient? Do I need any other process to be merged?

weiznich

Thank for the update. This looks now much more like what I had in mind 👍

I've left a few more or less stylistic comments and some questions that I would like to see answered. Other than that I would like to see at least one test for the generate schema part as well.

Finally it would be great if you can open a PR for the web-page as well that documents this new configuration file extension there as well: https://github.com/sgrif/diesel.rs-website/blob/master/src/guides/configuring-diesel-cli.md

diesel_cli/tests/print_schema.rs

diesel_cli/src/cli.rs

diesel_cli/src/config.rs

weiznich · 2024-02-09T12:14:33Z

diesel_cli/src/errors.rs

@@ -58,4 +58,8 @@ pub enum Error {
    ColumnLiteralParseError(syn::Error),
    #[error("Failed to parse database url: {0}")]
    UrlParsingError(#[from] url::ParseError),
+    #[error("Failed to parse CLI parameter: {0}")]
+    ClapMatchesError(#[from] clap::parser::MatchesError),
+    #[error("No print schema key with the name `{0}` exists")]


Can we point out more explicitly that this refers to [print_schema.{0}] entries in your diesel.toml file?

diesel_cli/src/migrations/mod.rs

diesel_cli/src/migrations/diff_schema.rs

diesel_cli/src/main.rs

diesel_cli/src/cli.rs

weiznich · 2024-02-09T12:23:28Z

(CI failures are caused by the recent rust release, I'm working on that)

Signed-off-by: forest1102 <[email protected]>

weiznich

This looks good now 👍. Thanks again for working on this. I will take care of the merge conflicts.

forest1102 · 2024-03-01T15:49:45Z

@weiznich
I am working on the merge conflict, so I will re-request review when it is done

weiznich · 2024-03-01T15:50:59Z

In that case: Can you also put an entry into the Changelog.md file for this change?

Signed-off-by: forest1102 <[email protected]>

weiznich · 2024-03-05T09:01:18Z

Thanks again for working on this 👍

forest1102 added 2 commits September 17, 2023 13:37

Support multiple schemas in diesel.toml

f304b19

Signed-off-by: forest1102 <[email protected]>

fix errors

7cda082

Signed-off-by: forest1102 <[email protected]>

forest1102 mentioned this pull request Sep 19, 2023

Support multiple schemas in diesel.toml #1728

Open

forest1102 added 2 commits September 20, 2023 00:58

Merge branch 'master' into forest1102/issue1728

1fcd745

Merge branch 'master' into forest1102/issue1728

5a9edd1

forest1102 changed the title ~~Support multiple schemas in diesel.toml~~ splitting into multiple schema.rs Sep 20, 2023

Merge branch 'master' into forest1102/issue1728

fb2df8c

forest1102 and others added 2 commits November 17, 2023 23:03

Merge branch 'master' into forest1102/issue1728

55e308b

Revert " Support multiple schemas in diesel.toml"

f9fa6a5

Signed-off-by: forest1102 <[email protected]>

forest1102 marked this pull request as draft December 4, 2023 14:32

Merge branch 'master' into forest1102/issue1728

d06b8aa

Merge branch 'master' into forest1102/issue1728

a23b136

forest1102 added 2 commits February 8, 2024 13:31

Merge branch 'master' into forest1102/issue1728

f2fe4a2

Merge branch 'master' into forest1102/issue1728

266a682

weiznich reviewed Feb 9, 2024

View reviewed changes

forest1102 and others added 2 commits February 25, 2024 18:27

Merge branch 'master' into forest1102/issue1728

57edc74

reflect suggestion

2860749

Signed-off-by: forest1102 <[email protected]>

forest1102 requested a review from weiznich February 26, 2024 07:24

Merge remote-tracking branch 'origin/master' into uforest1102/issue1728

2c21c43

Signed-off-by: forest1102 <[email protected]>

weiznich approved these changes Mar 1, 2024

View reviewed changes

forest1102 added 3 commits March 1, 2024 15:54

Merge remote-tracking branch 'origin/master' into forest1102/issue1728

5c2ae6d

changes after merge and refactor generate_sql_based_on_diff_schema

6458a46

Signed-off-by: forest1102 <[email protected]>

add changelog

455e863

Signed-off-by: forest1102 <[email protected]>

forest1102 requested a review from weiznich March 2, 2024 05:43

Merge branch 'master' into forest1102/issue1728

2343a5e

weiznich approved these changes Mar 5, 2024

View reviewed changes

weiznich enabled auto-merge March 5, 2024 09:01

weiznich added this pull request to the merge queue Mar 5, 2024

Merged via the queue into diesel-rs:master with commit 7c8cd02 Mar 5, 2024
48 checks passed

estk mentioned this pull request Mar 12, 2024

FR: Allow templating the output branche(s) of jj branch create jj-vcs/jj#3253

Open

BrewTestBot mentioned this pull request May 31, 2024

diesel 2.2.0 Homebrew/homebrew-core#173312

Closed

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

splitting into multiple schema.rs #3796

splitting into multiple schema.rs #3796

forest1102 commented Sep 17, 2023 •

edited

Loading

weiznich commented Sep 18, 2023

forest1102 commented Sep 19, 2023 •

edited

Loading

Sytten commented Sep 19, 2023

forest1102 commented Sep 20, 2023

Sytten commented Sep 20, 2023 •

edited

Loading

forest1102 commented Sep 20, 2023

Sytten commented Sep 20, 2023

forest1102 commented Sep 20, 2023

weiznich commented Sep 20, 2023

forest1102 commented Sep 20, 2023 •

edited

Loading

weiznich commented Sep 22, 2023

forest1102 commented Oct 5, 2023

weiznich commented Oct 6, 2023

forest1102 commented Oct 6, 2023 •

edited

Loading

weiznich commented Oct 9, 2023 •

edited

Loading

forest1102 commented Oct 11, 2023

weiznich commented Oct 11, 2023

forest1102 commented Oct 12, 2023 •

edited

Loading

weiznich commented Oct 12, 2023

forest1102 commented Feb 2, 2024

weiznich left a comment

weiznich Feb 9, 2024

weiznich commented Feb 9, 2024

weiznich left a comment

forest1102 commented Mar 1, 2024

weiznich commented Mar 1, 2024

weiznich commented Mar 5, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

splitting into multiple schema.rs #3796

splitting into multiple schema.rs #3796

Conversation

forest1102 commented Sep 17, 2023 • edited Loading

weiznich commented Sep 18, 2023

forest1102 commented Sep 19, 2023 • edited Loading

Sytten commented Sep 19, 2023

forest1102 commented Sep 20, 2023

Sytten commented Sep 20, 2023 • edited Loading

forest1102 commented Sep 20, 2023

Sytten commented Sep 20, 2023

forest1102 commented Sep 20, 2023

weiznich commented Sep 20, 2023

forest1102 commented Sep 20, 2023 • edited Loading

weiznich commented Sep 22, 2023

forest1102 commented Oct 5, 2023

weiznich commented Oct 6, 2023

forest1102 commented Oct 6, 2023 • edited Loading

weiznich commented Oct 9, 2023 • edited Loading

forest1102 commented Oct 11, 2023

weiznich commented Oct 11, 2023

forest1102 commented Oct 12, 2023 • edited Loading

weiznich commented Oct 12, 2023

forest1102 commented Feb 2, 2024

weiznich left a comment

Choose a reason for hiding this comment

weiznich Feb 9, 2024

Choose a reason for hiding this comment

weiznich commented Feb 9, 2024

weiznich left a comment

Choose a reason for hiding this comment

forest1102 commented Mar 1, 2024

weiznich commented Mar 1, 2024

weiznich commented Mar 5, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

forest1102 commented Sep 17, 2023 •

edited

Loading

forest1102 commented Sep 19, 2023 •

edited

Loading

Sytten commented Sep 20, 2023 •

edited

Loading

forest1102 commented Sep 20, 2023 •

edited

Loading

forest1102 commented Oct 6, 2023 •

edited

Loading

weiznich commented Oct 9, 2023 •

edited

Loading

forest1102 commented Oct 12, 2023 •

edited

Loading