Export by safer element's filename or id

[inazir]

Background

If someone puts special characters in the service dialog (for example) name, we will end up with special characters in filenames which will break our repository - at least if we import the repo onto a windows system. This issue is described in details here: rhtconsulting/cfme-rhconsulting-scripts#140

This pull request is supposed to generate a safer filename either by some string fields or by object id.
There is no handling of CustomButtons because safe_filename function is not called from there.

Links

• [Import/Export][Bug] Exporting alerts with faulty names #19655
• [Bug] Exporting alerts with wrong alert name  rhtconsulting/cfme-rhconsulting-scripts#140
• Export elements with id or guid instead of names during miqexport rhtconsulting/cfme-rhconsulting-scripts#149

[phospi]

1. We didn't fix the rubocop offenses about the literals because we didn't put the line there in the first place.
2. We don't know why the Travis build failed and what to do about it.
3. We didn't think about or tested the import with these changes so far.

[d-m-u]

What is object here?

[d-m-u]

I think we tend to prefer is_a? or kind_of?

[d-m-u]

This change makes me think that this maybe isn't the right way to do this.

[phospi]

Could you elaborate your thoughts? Whats the point in removing these attributes anyway? You can still throw them away while importing.

[d-m-u]

Hey @inazir, thanks for the PR. I've a couple comments but I've concerns about the approach.
As far as @phospi's comment goes, the rubocop offenses would still be great to look at (but I'd like to think more about the design of this PR before you have to worry about that), here's where the travis build failed: https://travis-ci.org/ManageIQ/manageiq/jobs/637488050#L707, and when this is a little more ironed out we will definitely need to test imports with these changes but too is that of secondary importance right now.

[miq-bot]

Checked commits inazir/manageiq@5bc8971~...cb01e28 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
17 files checked, 3 offenses detected

lib/task_helpers/exports/customization_templates.rb

• ❗ - Line 4, Col 23 - Style/PercentLiteralDelimiters - %i-literals should be delimited by [ and ].

lib/task_helpers/exports/provision_dialogs.rb

• ❗ - Line 4, Col 23 - Style/PercentLiteralDelimiters - %i-literals should be delimited by [ and ].

lib/task_helpers/exports/roles.rb

• ❗ - Line 4, Col 23 - Style/PercentLiteralDelimiters - %w-literals should be delimited by [ and ].

[phospi]

Hey @d-m-u, could you elaborate your concerns? We're currently very concerned about the state of the export/import function as well.

[d-m-u]

I'd rather see this added as functionality that's part of the optimist options so you could specify something like --sanitize-name Sanitize filenames in https://github.com/ManageIQ/manageiq/blob/master/lib/task_helpers/exports.rb#L10 and then use activesupport's parameterize if that option was passed in rather than this. It reduces this to about ten lines. It will need to be tested in order to be merged. Also, please in the future squash your commits.

[phospi]

I'd rather see this added as functionality that's part of the optimist options so you could specify something like --sanitize-name Sanitize filenames in https://github.com/ManageIQ/manageiq/blob/master/lib/task_helpers/exports.rb#L10 and then use activesupport's parameterize if that option was passed in rather than this. It reduces this to about ten lines. It will need to be tested in order to be merged.

We already built our implementation upon optimist with a parameter called "super_safe_filenames". The funny thing is that every export is currently handled by a function ".safe_filename" which sadly does not produce properly safe filenames.

So, you mean, you would prefer activesupport's parameterize for filename generation and sanitation instead of the proposed implementation of manageiq id's?

Are you aware that parameterize is supposed to sanitize url's and not filenames? I do not understand why an ugly textstring which could never be used for re-importing should be taken as filename. Your argument is that less code is needed. Did you think about edge cases?

• What happens if a description is longer than 255 characters? --> We have to truncate. (I know, easy to implement.)
• What happens if two descriptions are exactly the same? --> We will get a file exists error if we do not implement additional handling. (I'd say, not so easy to implement.)

My point is: relying on a non-unique text string for creating filenames is not a good idea. Is using mangeiq id's the best thing to do? I definitely don't know and I guess not but it ensures a consistent and unique and as far as I can tell safer naming for exported files.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[miq-bot]

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.