Tags without a classification cause errors

[juliancheal]

This fixes BZ https://bugzilla.redhat.com/show_bug.cgi?id=1632901

[juliancheal]

@miq-bot add_label bug, gaprindashvili/yes

[gtanzillo]

@juliancheal This looks good but it would be best to add a quick test with a tag without a classification before merging

[juliancheal]

@gtanzillo Yes sure, meant to ask you about that yesterday.

[JPrause]

@miq-bot add_label blocker

[JPrause]

@juliancheal please also add the hammer/yes label

[juliancheal]

@miq-bot add_label hammer/yes

[Fryguy]

Higher level question... Why would a tag not have a classification? That seems like a case that shouldnt happen.

cc @kbrock as I know you've looked at this from a performance perspective before

[Fryguy]

@juliancheal can you also fix the rubocop issue?

[juliancheal]

Higher level question... Why would a tag not have a classification? That seems like a case that shouldnt happen.

@Fryguy Right, we shouldn't but from the BZ they do.

[miq-bot]

Checked commits juliancheal/manageiq@7a2aad3~...8bb45d6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

[juliancheal]

To summarise the offline conversation I had with @gtanzillo. The customer has a situation where they have Tags that return true for tag.show which means the code reaches this else statement https://github.com/ManageIQ/manageiq/blob/master/app/models/tag.rb#L148

However at the same time, there is no classification.name or classification.description. So the code would crash by calling a method on nil.

This "fix" makes it so the code doesn't 💣 out, but it would be good to find the reason we're in this situation.

/cc @Fryguy @kbrock

[lpichler]

Higher level question... Why would a tag not have a classification? That seems like a case that shouldnt happen.

I was facing with this cases on CUs's DBs also (#17883)

[juliancheal]

🍏 @gtanzillo

[kbrock]

@Fryguy ugh - late to the party
Yes, all tags need to have classifications for them to work.
This is a bug in the database, not in our code.

Maybe the custom attribute stuff?

[simaishi]

Hammer backport details:

$ git log -1
commit bac84088f5b756e7d136dd8746612defa93c3487
Author: Gregg Tanzillo <[email protected]>
Date:   Fri Nov 9 13:35:27 2018 -0500

    Merge pull request #18177 from juliancheal/fix_tags_without_classifications_errors
    
    Tags without a classification cause errors
    
    (cherry picked from commit 3217a89bb0d0c76d7a482eada25e93e4f35ae2f4)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1632901

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 50d3412e5425fb46a93fb2afed837f30d3ab2c17
Author: Gregg Tanzillo <[email protected]>
Date:   Fri Nov 9 13:35:27 2018 -0500

    Merge pull request #18177 from juliancheal/fix_tags_without_classifications_errors
    
    Tags without a classification cause errors
    
    (cherry picked from commit 3217a89bb0d0c76d7a482eada25e93e4f35ae2f4)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1648948

[jrafanie]

Shouldn't this have been @tag.classification.delete?

If I do this and issue @tag.reload, I get:

  1) Tag categorization tag with nil classification
     Failure/Error: expect(@tag.show).to eq(true)

       expected: true
            got: nil

       (compared using ==)
     # ./spec/models/tag_spec.rb:117:in `block (3 levels) in <top (required)>'

Note, I'm here because rails 5.0 looks to have been returning a cached classification when @tag.categorization calls @tag.category in show. Because this was failing, I changed my tests locally to do @tag.classification.delete and it fails as shown above in both rails 5.0 and 5.1.

I'm not sure how any of the @category expectations are set, such as @category.name below:
To get to the category, you need to navigate through using @tag.classification.parent (what category does I think). If the classification is nil or deleted, category should also be nil, which would cause show to be nil.

😕

[jrafanie]

In other words, I believe we're relying on @tag.category to return the cached parent of the classification's parent. If you delete the classification, it fails. If you upgrade to rails 5.1, the classification is nil so the category (classification parent) is also nil.

I believe this test is wrong. I think.