Fixes #36017 - Remove spacing from array type settings #9614

girijaasoni · 2023-02-08T07:44:24Z

…ts to make it into a valid format

theforeman-bot · 2023-02-08T07:44:27Z

Issues: #36017

theforeman-bot · 2023-02-08T07:44:27Z

Can one of the admins verify this patch?

theforeman-bot · 2023-02-08T07:44:27Z

Can one of the admins verify this patch?

theforeman-bot · 2023-02-08T07:44:27Z

Can one of the admins verify this patch?

stejskalleos · 2023-02-08T08:12:15Z

ok to test

Ron-Lavi

Thanks @girijaasoni !
the value which is stored in the DB still has spaces,
can we also trim the spaces from it?

Ron-Lavi

We should consider trimming the values from the frontend, e.g:

foreman/webpack/assets/javascripts/react_app/components/SettingUpdateModal/components/SettingForm/SettingForm.js

Line 26 in 662c4a3

values: submitValues,

Though to better protect the DB also from the API point, maybe we can use this library: https://github.com/holli/auto_strip_attributes

this can be a solution for more scenarios across Foreman and plugins...
interested in what @nofaralfasi and @theforeman/packaging have to say about it.

ekohl

I'm with @Ron-Lavi that I'd avoid changing the validators, but rather make sure the input is clean. As @Ron-Lavi pointed out, you will end up with more complex values in the DB which may confuse other code later on and create subtle bugs.

ekohl · 2023-02-13T11:48:26Z

app/validators/array_type_validator.rb

@@ -1,5 +1,5 @@
 class ArrayTypeValidator < ActiveModel::EachValidator
  def validate_each(record, attribute, value)
-    record.errors.add attribute, _("must be comma separated") unless Array(value).map { |item| item.to_s.match /\A\S+\z/ }.all?
+    record.errors.add attribute, _("must be comma separated") unless Array(value).map { |item| item.to_s.strip.match /\A\S+\z/ }.all?


Unrelated to this PR, but I think .all? accepts a block and this could be optimized:

Suggested change

record.errors.add attribute, _("must be comma separated") unless Array(value).map { |item| item.to_s.strip.match /\A\S+\z/ }.all?

record.errors.add attribute, _("must be comma separated") unless Array(value).all? { |item| item.to_s.strip.match?(/\A\S+\z/) }

Note I didn't test this and it's probably better to do in a separate PR

To be explicit: the changes to the validators should be reverted. They should not accept any whitespace.

@ekohl Are you suggesting that we shall apply the change somewhere else or we can modify the Regex for the same validation?

I'd apply the change elsewhere. Normally the flow is:

User input

Apply normalization

Apply validators

Process data

Processing data can mean storing it in the DB, but you want it to be as clean as possible. The main reason is that if you can assume that data in the database doesn't have whitespace around the value, you can keep the code that uses data from the database simpler. Or put in another way: if you must remember to always call .strip on a value, you can easily introduce bugs. That's more important than the potential performance difference, but that's another factor.

An example:

if model.value == 'value' # Do X else # Do Y end

This will work most of the time, but if the user (accidentally) has spaces in the value it can introduce a bug.

Looking back at the flow, this means we want to clean up the data before processing it. A validator is then only supposed to validate. It never changes data. This means normalization can happen in the UI, before it's submitted, or in the controller before it's passed to the validators. But the validator must remain strict.

Thanks a lot for the clarification Ewoud!!

ekohl

To be explicit: the changes to the validators should be reverted. They should not accept any whitespace.

nofaralfasi · 2023-03-01T12:38:46Z

@girijaasoni Some tests are failing.
And can you also update the commit message to reflect your new changes?

Ron-Lavi

Thanks @girijaasoni, I tested and it works as expected now 👍🏼

Ron-Lavi · 2023-04-02T11:45:52Z

test/unit/oidc_jwt_validate_test.rb

@@ -44,13 +44,13 @@ def setup
    end

    test 'if audience is not valid' do
-      Setting['oidc_audience'] = "no-client"
+      Setting['oidc_audience'] = ["no-client"]


Nice catch, it was indeed moved to setting_type of array in

foreman/db/migrate/20200326164755_change_oidc_audience_setting_type.rb

Line 6 in 7907787

setting.settings_type = 'array'

girijaasoni · 2023-04-03T08:54:16Z

@ekohl , could you please review the updated changes?

ekohl

Changes look good to me. It would be great to have a test, but I don't know where would be a great place.

stejskalleos · 2023-04-14T10:41:33Z

test/models/setting_test.rb

+    assert Setting.create(name: 'trusted_hosts', value: [])
+    setting = Setting.find_by_name("trusted_hosts")
+    setting.value = ["localhost ", "  remotehost"]
+    assert setting.save


setting = Setting.create(name: 'trusted_hosts', value: ["localhost ", " remotehost"]) assert_equal ["localhost", "remotehost"], setting.value

Thank you @stejskalleos

stejskalleos · 2023-04-14T10:43:01Z

test/models/setting_test.rb

@@ -174,6 +174,13 @@ def test_settings_should_save_strings
    assert_equal "must be comma separated", setting.errors[:value].first
  end

+  test "trusted_hosts should accept values with spaces" do


trusted_hosts should trim values with spaces

https://www.betterspecs.org/#should says you should make it:

Suggested change

test "trusted_hosts should accept values with spaces" do

test "trusted_hosts accepts values with spaces" do

Granted, that's for RSpec, but I like the convention.

Thanks @ekohl , updated it

stejskalleos

🍏 LGTM

Failed tests are not related, but let's wait with the merge until they are fixed.

girijaasoni · 2023-04-19T11:48:20Z

green_apple LGTM

Failed tests are not related, but let's wait with the merge until they are fixed.

The tests pass now!

stejskalleos

🍏 LGTM

Works as expected.
Thanks @girijaasoni!

theforeman-bot added Waiting on contributor Not yet reviewed labels Feb 8, 2023

girijaasoni force-pushed the 36017 branch from 789ccf1 to 1d5c362 Compare February 8, 2023 11:09

theforeman-bot removed the Waiting on contributor label Feb 8, 2023

girijaasoni changed the title ~~Fixes #36017 -Used .strip to solve space and comma issue between inpu…~~ Fixes #36017 - .strip for input validation Feb 8, 2023

girijaasoni changed the title ~~Fixes #36017 - .strip for input validation~~ Fixes #36017 - Input validation to accept spaces Feb 8, 2023

Ron-Lavi self-assigned this Feb 8, 2023

Ron-Lavi reviewed Feb 8, 2023

View reviewed changes

theforeman-bot added the UI label Feb 13, 2023

ekohl requested changes Feb 13, 2023

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Feb 13, 2023

girijaasoni closed this Feb 15, 2023

girijaasoni force-pushed the 36017 branch from 4919ddd to 9519609 Compare February 15, 2023 12:57

theforeman-bot added Needs re-review and removed Waiting on contributor labels Feb 15, 2023

girijaasoni reopened this Feb 15, 2023

ekohl requested changes Feb 15, 2023

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Feb 15, 2023

girijaasoni force-pushed the 36017 branch from 29b649e to 6a0fbc9 Compare March 1, 2023 12:04

theforeman-bot added Needs re-review and removed Waiting on contributor labels Mar 1, 2023

girijaasoni force-pushed the 36017 branch 2 times, most recently from 12c5d0e to f556ff3 Compare March 30, 2023 11:44

Ron-Lavi previously approved these changes Apr 2, 2023

View reviewed changes

theforeman-bot removed the Needs re-review label Apr 2, 2023

ekohl previously approved these changes Apr 3, 2023

View reviewed changes

girijaasoni dismissed stale reviews from ekohl and Ron-Lavi via 16270dd April 14, 2023 09:58

girijaasoni force-pushed the 36017 branch from f556ff3 to 16270dd Compare April 14, 2023 09:58

stejskalleos reviewed Apr 14, 2023

View reviewed changes

girijaasoni force-pushed the 36017 branch 2 times, most recently from 1955637 to e1d7bcc Compare April 14, 2023 11:52

girijaasoni closed this Apr 14, 2023

girijaasoni force-pushed the 36017 branch from e1d7bcc to 56fe651 Compare April 14, 2023 11:57

girijaasoni reopened this Apr 14, 2023

girijaasoni force-pushed the 36017 branch from 367d718 to 932203f Compare April 14, 2023 12:10

stejskalleos previously approved these changes Apr 19, 2023

View reviewed changes

Fixes #36017 - Remove spacing for array type settings

4e0d39c

girijaasoni dismissed stejskalleos’s stale review via 4e0d39c April 19, 2023 11:37

girijaasoni force-pushed the 36017 branch from 932203f to 4e0d39c Compare April 19, 2023 11:37

girijaasoni requested review from stejskalleos, Ron-Lavi, ekohl and evgeni April 19, 2023 11:47

nofaralfasi approved these changes Apr 19, 2023

View reviewed changes

stejskalleos approved these changes Apr 20, 2023

View reviewed changes

stejskalleos merged commit 6368162 into theforeman:develop Apr 20, 2023
12 checks passed

MariaAga added the Needs cherrypick This should be cherrypicked to the stable branches or branches label Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #36017 - Remove spacing from array type settings #9614

Fixes #36017 - Remove spacing from array type settings #9614

girijaasoni commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

stejskalleos commented Feb 8, 2023

Ron-Lavi left a comment

Ron-Lavi left a comment

ekohl left a comment

ekohl Feb 13, 2023

girijaasoni Feb 20, 2023

ekohl Feb 20, 2023

nofaralfasi Feb 21, 2023

ekohl left a comment

nofaralfasi commented Mar 1, 2023 •

edited

Ron-Lavi left a comment

Ron-Lavi Apr 2, 2023

girijaasoni commented Apr 3, 2023

ekohl left a comment

stejskalleos Apr 14, 2023 •

edited

girijaasoni Apr 14, 2023

stejskalleos Apr 14, 2023

ekohl Apr 14, 2023

girijaasoni Apr 14, 2023

stejskalleos left a comment

girijaasoni commented Apr 19, 2023 •

edited

stejskalleos left a comment

	record.errors.add attribute, _("must be comma separated") unless Array(value).map { \|item\| item.to_s.strip.match /\A\S+\z/ }.all?
	record.errors.add attribute, _("must be comma separated") unless Array(value).all? { \|item\| item.to_s.strip.match?(/\A\S+\z/) }

	test "trusted_hosts should accept values with spaces" do
	test "trusted_hosts accepts values with spaces" do

Fixes #36017 - Remove spacing from array type settings #9614

Fixes #36017 - Remove spacing from array type settings #9614

Conversation

girijaasoni commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

theforeman-bot commented Feb 8, 2023

stejskalleos commented Feb 8, 2023

Ron-Lavi left a comment

Choose a reason for hiding this comment

Ron-Lavi left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

nofaralfasi commented Mar 1, 2023 • edited

Ron-Lavi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girijaasoni commented Apr 3, 2023

ekohl left a comment

Choose a reason for hiding this comment

stejskalleos Apr 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stejskalleos left a comment

Choose a reason for hiding this comment

girijaasoni commented Apr 19, 2023 • edited

stejskalleos left a comment

Choose a reason for hiding this comment

nofaralfasi commented Mar 1, 2023 •

edited

stejskalleos Apr 14, 2023 •

edited

girijaasoni commented Apr 19, 2023 •

edited