New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to rename table columns in table! macro and do it automatically for invalid names #967

Closed
Fiedzia opened this Issue Jun 26, 2017 · 20 comments

Comments

Projects
None yet
6 participants
@Fiedzia
Contributor

Fiedzia commented Jun 26, 2017

I have a table with a column named "mod", which is not valid identifier in Rust.
Even for valid identifiers, I want to be able to use different name than what database provides,
is that possible?

Same thing should be applied to table names.

@killercup

This comment has been minimized.

Member

killercup commented Jun 26, 2017

Yes, this is something we want to allow. There is a previous discussion (probably outdated a bit by now) in #424.

@Fiedzia do you have a concrete suggestion for the syntax to rename columns in table!?

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jun 26, 2017

The naive solution would be to add some form of "real name" as foo -> Integer,
but ideally table! enhancement should take into account other things one may want to set there.

I consider Django ORM to be a golden standard in this regard because it incorporates
a lot of domain knowledge in this aspect (among other reasons), and for example here are some of the things it allows to set in field definition:

  • nullability
  • can the value be blank
  • choices (this one is also important for me)
  • column name
  • if an index should be created for this field (multifield indexes are property of a table)
  • tablespace
  • default value
  • if it is primary key (multifield indexes are property of a table)
  • human-readable field name
  • validators
  • (some other django-related bits I'll ignore here)

as well as properties specific for particular field type:

  • precision (for decimal fields)
  • max_length for text fields

https://docs.djangoproject.com/en/1.11/ref/models/fields/

Perhaps not all of it makes sense for Diesel, but I'd prefer to have generic and extensible solution for setting field and table attributes.

As for syntax, I'd suggest something similar to Django:

table! {
    users {
        id -> Integer(primary_key: true),
        name -> VarChar(db_column: "username", max_length:120),
        properties:
            indexes: ...
            db_table: "user"
    }
}

(I find ":" more readable than "=")

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 4, 2017

Let's keep the feature focused. The use case we want to support here is renames, we don't need to complicate things by adding a bunch of other information that Diesel does not care about (and likely never will).

I'm more in favor of using attributes for this, but want to make sure the naming makes it very clear what is the Rust side and what is the SQL side.

table! {
    users {
        id -> Integer,
        #[database_name="username"]
        name -> Text
    }
}

I'd also like to see a concrete proposal for the conventional renames for Rust keywords.

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 4, 2017

#[database_name="username"]

I don't mind the format for setting attributes, but I'd prefer to call this attribute column_name or database_column_name - that makes clear what it does even for someone not familiar with diesel.

I'd also like to see a concrete proposal for the conventional renames for Rust keywords.

Django adds _field sufix in this case. Since Diesel uses column name, that would be _column.
https://github.com/django/django/blob/master/django/core/management/commands/inspectdb.py#L202
However, this is inconsistent with column names being numbers, which need to prefixed, so perhaps just always prefixing would be better. Therefore my proposal would be:

"mod" => column_mod
"12345" => column_12345

I'd be happy if whole normalization (and adding comments to explain it) would be stolen from Django.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 4, 2017

I'd rather not establish a convention for column names containing only numbers (or any other column which is not valid as an unquoted identifier in SQL)

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 4, 2017

You will need to have some convention, interact with user or generate code that doesn't compile (as it is now). Of those three, I'd definitely prefer convention. Interacting with database tool for some large database is not something I'd enjoy doing. I am working with a system that has dozens of databases and thousands of tables. I have no idea how many of those are not valid identifiers in Rust, but I do not want to go through all of them and make a decision how it should be renamed for every case.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 4, 2017

Why should we special case numbers but not spaces? Or other characters that aren't valid identifiers? We would have to conventionally handle any arbitrary sequence of characters. infer_schema! does not need to handle absolutely every possible edge case, as long as we provide the tools for users to make it work on their own. Providing renames does that.

That said, the issue with Rust keywords (notably type) is incredibly common, and a frequent source of friction that does warrant special casing.

I have no idea how many of those are not valid identifiers in Rust

Have you tried throwing infer_schema! at it? I'd be interested to hear how many of these cases you have

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 4, 2017

Why should we special case numbers but not spaces?

My opinion is that Diesel should handle everything - I've linked to Django as it covers a lot of cases (I am not sure if all, but many more than just numbers and keywords).

infer_schema! does not need to handle absolutely every possible edge case, as long as we provide the tools for users to make it work

The problem is that this can be extremely tedious and user-unfriendly work. There is no point in users doing it at all.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 4, 2017

There is no point in users doing it at all.

I agree, which is why the overwhelming majority of the time, you won't see column names that aren't valid SQL identifiers.

I don't see the value of adding a convention for breaking convention, especially one that's so uncommon.

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 4, 2017

I agree, which is why the overwhelming majority of the time, you won't see column names that aren't valid SQL identifiers.

They do exist though, I can't ignore that, and I want the tools I am using to deal with that without involving me. I do not want for example to break my build scripts just because someone decided to add some column that Rust doesn't like. This is trivial problem to solve and I see no reason why a database tool should ever expose such issues to users. Perhaps look at this in context of creating web project as a whole, which includes using multiple tools. Preferably all of them should work as smoothly and automatically as it is possible.

@sgrif

This comment has been minimized.

Member

sgrif commented Jul 4, 2017

I'm not claiming that they don't exist, I'm saying that they're not common enough to warrant any sort of convention for automatically renaming them, especially since columns that are invalid SQL identifiers are by definition breaking convention. The issue is already going to be exposed to the user when they have to figure out what we renamed their column to.

Anyway we're not getting anywhere by debating that further. I think you've made your position very clear. What I'd like to move forward with for the time being is an annotation to allow renames (something like database_column_name sounds good I think), and then a separate PR that introduces the conventional column renames for Rust keywords. I'm not sold on column_ as a prefix, but we can discuss that in the PR.

@Fiedzia

This comment has been minimized.

Contributor

Fiedzia commented Jul 4, 2017

Great, thanks.

@killercup

This comment has been minimized.

Member

killercup commented Jul 11, 2017

Also interesting: Column names that collide with type names that are in scope: #1018

@Measter

This comment has been minimized.

Measter commented Jul 11, 2017

Apologies for missing this issue; apparently my google-foo isn't as good as I'd hoped.

In terms of re-name syntax, I'm not massively familiar with writing macros, but is it possible to do something like this:

table! {
    users {
        id -> Integer,
        name as username -> Text
    }
}
@sgrif

This comment has been minimized.

Member

sgrif commented Jul 12, 2017

@Measter It is possible, but I do not like that syntax. Which side is the rust side? Which is the SQL side? I'd strongly prefer something that uses an attribute (for consistency with Rust's syntax), and makes it clear in the name which side is which.

@Eijebong

This comment has been minimized.

Member

Eijebong commented Jul 12, 2017

@sgrif And I think it wouldn't look good with keywords (if it even works), type as type_ vs

#[column_name="type"]
type_
@sgrif

This comment has been minimized.

Member

sgrif commented Jul 13, 2017

Agreed

@Eijebong

This comment has been minimized.

Member

Eijebong commented Aug 24, 2017

Fixed by #1084 and #1110

@Eijebong Eijebong closed this Aug 24, 2017

@kardeiz

This comment has been minimized.

Contributor

kardeiz commented Sep 27, 2017

I don't think this issue should have been closed. The initial poster specifies:

Same thing should be applied to table names.

But the fix in #1084 only applies to column names. I also am interested in a fix that applies to table names as well.

@sgrif

This comment has been minimized.

Member

sgrif commented Sep 27, 2017

Can you open a new issue for renaming table names?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment