Skip to content
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

Deprecate invalid custom attribute names #18538

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Mar 8, 2019

add_custom_attribute/miq_custom_set will now log a deprecation if we try
to define or add a custom attribute name that is an invalid column name.

https://bugzilla.redhat.com/show_bug.cgi?id=1662319

From:
https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS

SQL identifiers and key words must begin with a letter (a-z, but also
letters with diacritical marks and non-Latin letters) or an underscore
(_). Subsequent characters in an identifier or key word can be letters,
underscores, digits (0-9), or dollar signs ($). Note that dollar signs
are not allowed in identifiers according to the letter of the SQL
standard, so their use might render applications less portable. The SQL
standard will not define a key word that contains digits or starts or
ends with an underscore, so identifiers of this form are safe against
possible conflict with future extensions of the standard.

Similar to https://bugzilla.redhat.com/show_bug.cgi?id=1558618

add_custom_attribute/miq_custom_set will now log a deprecation if we try
to define or add a custom attribute name that is an invalid column name.

https://bugzilla.redhat.com/show_bug.cgi?id=1662319

From:
https://www.postgresql.org/docs/current/static/sql-syntax-lexical.html#SQL-SYNTAX-IDENTIFIERS

SQL identifiers and key words must begin with a letter (a-z, but also
letters with diacritical marks and non-Latin letters) or an underscore
(_). Subsequent characters in an identifier or key word can be letters,
underscores, digits (0-9), or dollar signs ($). Note that dollar signs
are not allowed in identifiers according to the letter of the SQL
standard, so their use might render applications less portable. The SQL
standard will not define a key word that contains digits or starts or
ends with an underscore, so identifiers of this form are safe against
possible conflict with future extensions of the standard.

Similar to https://bugzilla.redhat.com/show_bug.cgi?id=1558618
@miq-bot miq-bot added the wip label Mar 8, 2019
@jrafanie
Copy link
Member Author

jrafanie commented Mar 8, 2019

@Ladas @agrare can you review? 🙇 to @bdunne for the initial attempt via #17372

It warns if you use invalid characters in column names. I'd assume, you'd have to also check this on the way in during refresh. How would you want this exposed?

@miq-bot
Copy link
Member

miq-bot commented Mar 8, 2019

Checked commit jrafanie@eaad9fb with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

spec/models/mixins/custom_attribute_mixin_spec.rb

@jrafanie jrafanie changed the title [WIP] Deprecate invalid custom attribute names Deprecate invalid custom attribute names Mar 15, 2019
@jrafanie jrafanie requested a review from agrare March 15, 2019 15:01
@jrafanie jrafanie removed the wip label Mar 15, 2019
@jrafanie jrafanie requested a review from tinaafitz March 15, 2019 15:01
@jrafanie
Copy link
Member Author

jrafanie commented Mar 15, 2019

Adding @agrare and @tinaafitz to review since this will now log deprecations on refreshes/automate that try to create custom attributes with "non-column" characters such as spaces. We will eventually want automate/providers refreshes to either reject these or clean them up so they can be used in reporting, expressions, UI, like any other column. Currently, custom attributes coming from refresh/automate with spaces do not work in all areas of the product.

@jrafanie
Copy link
Member Author

@tinaafitz @agrare please review, thanks!

Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrafanie Looks good.

@agrare
Copy link
Member

agrare commented Mar 19, 2019

Looks good to me, refresh doesn't use these accessors so we'll need to check these, cc @Ladas

@agrare agrare self-assigned this Mar 19, 2019
@agrare agrare merged commit eaad9fb into ManageIQ:master Mar 19, 2019
agrare added a commit that referenced this pull request Mar 19, 2019
…s_in_custom_attributes

Deprecate invalid custom attribute names
@jrafanie jrafanie deleted the deprecate_invalid_column_names_in_custom_attributes branch March 19, 2019 13:06
@JPrause
Copy link
Member

JPrause commented Mar 20, 2019

@jrafanie if this can be backported, can you add the hammer/yes label.

simaishi pushed a commit that referenced this pull request Mar 29, 2019
…s_in_custom_attributes

Deprecate invalid custom attribute names

(cherry picked from commit a26aa9d)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693722
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 14e100cf68085524e229ccdf34fe8acdfa82d85d
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Mar 19 11:42:19 2019 +0100

    Merge pull request #18538 from jrafanie/deprecate_invalid_column_names_in_custom_attributes
    
    Deprecate invalid custom attribute names
    
    (cherry picked from commit a26aa9ddbbb52e9b2027c199cd7782fc8a68c8cb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1693722

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

Successfully merging this pull request may close these issues.

6 participants