refactor RuleConfig + overriding default values #105

ankithans · 2021-07-01T10:35:46Z

Closes #104

Description

Now Rule Config can be overridden by the user

refactor ruleConfig and rules interface
overriding of rules by the user

Type of change

Bug fix (non-breaking change which fixes an issue)
Documentation change

wtrocki · 2021-07-01T10:40:23Z

validator/rules/executor.go

-	var verbose bool
-	if config != nil && config.Verbose {
-		verbose = true
+	if err := mergo.Merge(&config.Length, defaultConfig.Length); err != nil {


Why we would merge specific rules. Can we merge all config paths?

wtrocki · 2021-07-01T10:42:17Z

validator/rules/executor.go

-	var verbose bool
-	if config != nil && config.Verbose {
-		verbose = true
+	if err := mergo.Merge(&config.Length, defaultConfig.Length); err != nil {


I think overal config is confusing. WE should either:

Add rules field with array of IRule

Put string Rule after each rule(not like it personally)

okay, so I really need to rethink the architecture, as everything is dependent on config. I will refactor it, keeping the functionality same!

I have one more concern - here we don't really need validator package, shall we do everything under rules package? But this can be changed while doing above refactoring!

Yes.. No pressure. Take it easy

If the aim here is to merge two config files into one giving precedence to a passed in file but keeping default values were no values are passed in and we are already using mergo. We could try using merge with the transformer: mergo.Merge(&config.Length, defaultConfig.Length, mergo.WithOverride). This should keep config with first values whilst only updating fields passed in from defaultConfig.Length.

Speaking of .Length is this the best name for this field? is it a validated ( element size wise ) map of config fields? I may just not be getting it either, in which case my suggestion may not be applicable.

I got you point @dimakis. But we want to keep defaultConfig.Length as it is and merge config.Length into it. We can do opposite as you suggested but then we need to do *config = *defaultConfig, extra lines of code. This can be avoided I'd say. And btw this code might change as there is some major refactoring going on.

Speaking of .Length is this the best name for this field? is it a validated ( element size wise ) map of config fields? I may just not be getting it either, in which case my suggestion may not be applicable.

Length is a struct of Limits (that is map[string]Limit) and Limit is struct consisting of Max, Min length of field(int value). We can think over the names again.

Why we would merge specific rules. Can we merge all config paths?

Problem was mergo cannot merge 2 slices. It can only merge structs and maps. And the previous method also had similar problems.

I got you point @dimakis. But we want to keep defaultConfig.Length as it is and merge config.Length into it. We can do opposite as you suggested but then we need to do *config = *defaultConfig, extra lines of code. This can be avoided I'd say. And btw this code might change as there is some major refactoring going on.

It may be one extra assignment, but it negates the need to check for duplication of values in a slice -> removeDuplicates() becomes obsolete.

+1 on trying to find better fitting names

dimakis

great work @ankithans 👍

dimakis · 2021-07-02T01:25:36Z

validator/rules/executor.go

-	var verbose bool
-	if config != nil && config.Verbose {
-		verbose = true
+	if err := mergo.Merge(&config.Length, defaultConfig.Length); err != nil {


If the aim here is to merge two config files into one giving precedence to a passed in file but keeping default values were no values are passed in and we are already using mergo. We could try using merge with the transformer: mergo.Merge(&config.Length, defaultConfig.Length, mergo.WithOverride). This should keep config with first values whilst only updating fields passed in from defaultConfig.Length.

Speaking of .Length is this the best name for this field? is it a validated ( element size wise ) map of config fields? I may just not be getting it either, in which case my suggestion may not be applicable.

wtrocki · 2021-07-02T21:34:49Z

Is that ready for review?

ankithans · 2021-07-03T01:47:20Z

Not yet, I will ping you, fixing an issue after refactor.

ankithans · 2021-07-03T06:45:23Z

One thing left in this PR ie. overriding. I am stuck and trying to figure out the way to do this! Mergo cannot be used.

Merge this

RuleConfig{
	Rules: []rules.Rules{
		&rules.Length{Verbose: true, Limits: map[string]rules.Limit{"Use": {Min: 100}}},
	},
}

into this

RuleConfig{
	Rules: []Rules{
		&Length{
			Verbose: false,
			Limits: map[string]Limit{
				"Use":     {Min: 2},
				"Short":   {Min: 15},
				"Long":    {Min: 50},
				"Example": {Min: 50},
			}},
		&MustExist{Verbose: false, Fields: []string{"Use", "Short", "Long", "Example"}},
	},
}

Result should be

RuleConfig{
	Rules: []Rules{
		&Length{
			Verbose: true,
			Limits: map[string]Limit{
				"Use":     {Min: 100},
				"Short":   {Min: 15},
				"Long":    {Min: 50},
				"Example": {Min: 50},
			}},
		&MustExist{Verbose: false, Fields: []string{"Use", "Short", "Long", "Example"}},
	},
}

dimakis · 2021-07-03T07:23:29Z

@ankithans do you have an example of how you are trying to use it? can you push it so i can have a look?

ankithans · 2021-07-03T07:25:26Z

@dimakis this is the example I am using for testing https://github.com/aerogear/charmil/tree/ankithans/issue104/validator/example

and here we need to merge defaultConfig & config(given by user) https://github.com/aerogear/charmil/blob/ankithans%2Fissue104/validator/rules/executor.go#L110

dimakis · 2021-07-03T07:40:25Z

validator/rules/executor.go

-			log.Fatal(errr)
-		}
-	}
+	// TODO: Override default configuration


@ankithans All I'm seeing is an empty TODO, have/ can you push up your implementation?

sure give me sometime, I will ping you once done. i want to push some working code.

It's not essential for it to be working, just change the PR to a [WIP]. I enjoy that you don't want to push non functional code, but if you are still facing issues push it and I'll see if I can help. Don't get too hung up on a single piece of the puzzle. 👍

thank you so much!!!
Now Length rule is working fine! as it has a map field Limits (map[string]Limit) (Limit -> {Min, Max int}) to merge
but MustExist cannot be merged with mergo as it is a slice. One solution is to convert

Fields []string

to

Fields map[string]bool

what do you suggest?

I did it this way, changed the Fields []string to Fields map[string]bool. This way user can disable the default MustExist fields(which was not possible previously)
and easy to merge with mergo 😀

ankithans · 2021-07-03T08:42:38Z

@wtrocki @dimakis ready for review

wtrocki · 2021-07-03T10:54:14Z

@ankithans configuration and defaults are common problem. Please reseach this topic first.

1 min Google returned me this:
https://github.com/containous/staert

Although I'm not fan of libraries

Please check how golangci lint does it
https://github.com/golangci/golangci-lint

wtrocki · 2021-07-03T10:55:33Z

Good start is to query google properly:
Example:

https://www.google.com/search?q=golang+struct+default+value&client=ms-android-xiaomi-rvo3&ei=jkHgYLm4CYeT8gL4p5zQAQ&oq=golang+structure+defaults&gs_lcp=ChNtb2JpbGUtZ3dzLXdpei1zZXJwEAEYADIICAAQFhAKEB46BAgAEEc6AggpOgUIABCRAjoCCAA6BAgAEA06BggAEBYQHjoFCCEQoAFQiE9Y2HxgxYYBaANwAXgAgAFqiAHMEZIBBDMyLjGYAQCgAQHIAQjAAQE&sclient=mobile-gws-wiz-serp

ankithans · 2021-07-03T11:27:30Z

@wtrocki How I did this now is according to #110
User API looks like this, as you and @jackdelahunt also suggested.

vali := rules.RuleConfig{
	Verbose: false,
	Rules: []rules.Rules{
		&rules.Length{Limits: map[string]rules.Limit{"Use": {Min: 2}}},
		&rules.MustExist{Fields: map[string]bool{"Long": false}},
	},
}

And we have solved the merging of the default config & user provided config in efficient way, using mergo
https://github.com/aerogear/charmil/blob/ankithans%2Fissue104/validator/rules/executor.go#L96

if err := mergo.Merge(&defaultConfig, config, mergo.WithSliceDeepCopy); err != nil {
	log.Fatal(err)
}
*config = defaultConfig

seen golangci-lint configuration file, it also accepts diff config for each linter.
Also got much help from this issue opened in mergo a long time back darccio/mergo#121 Many were facing the same issue as I was!

wtrocki · 2021-07-03T11:47:04Z

I think problem is that we trying doing something different:

We doing merge that is tricky in go, but can do default values for structure which is supported by go core (with json) or covered by some libs

ankithans · 2021-07-03T12:55:25Z

Okay, I got your point now! But will it work with slice? need to initialize slice of Rule Interface.
How will it work with Min in length, as that will be different for each attribute in cobra.
I think it will be better to initialize manually in the array of rules, what do you think?

wtrocki · 2021-07-03T17:32:40Z

Just have all as structure.

Currenr structure have unneded complexity/slices etc.

Have structure that is parsable to json/yaml

Instead:

vali := rules.RuleConfig{
	Verbose: false,
	Rules: []rules.Rules{
		&rules.Length{Limits: map[string]rules.Limit{"Use": {Min: 2}}},
		&rules.MustExist{Fields: map[string]bool{"Long": false}},
	},
}

this

ValidatorConfig{
 RuntimeOptions,
Rules:{
 LenghtRule, 
 MustExist
}
}
LenghtRule{
  Max int32
  Min int32
}

KISS and DRY in action. Not sure why we overthinking it so much. If we write rule config as json golang can generate structure for you.

Witten on phone while feeding baby. Sorry for typos

ankithans · 2021-07-03T17:45:04Z

Witten on phone while feeding baby. Sorry for typos

No issues 😄, But I am bugging you again 😅

ValidatorConfig{
 RuntimeOptions,
Rules:{
 LenghtRule, 
 MustExist
}
}
LenghtRule{
  Max int32
  Min int32
}

If we follow above struct we would need to add rules in ruleExecutor manually, and that was what we wanted to solve. For maintainability we did choose array of Interface of Rules. Previously we were doing this only, accepting struct...

wtrocki · 2021-07-05T10:53:38Z

Rebase will solve this :D

validator/rules/config.go

wtrocki · 2021-07-05T10:57:54Z

validator/rules/config.go

+func ValidatorConfigToRuleConfig(validatorConfig *ValidatorConfig, ruleConfig *RuleConfig) {
+	defaultVerbose := validatorConfig.ValidatorOptions.Verbose
+
+	defaultConfigJson := `{


Wrong place to put default config :)

wtrocki · 2021-07-05T10:59:27Z

validator/rules/config.go

+		"ValidatorOptions": {
+			"Verbose": false
+		},
+		"ValidatorRules": {


The structure will be much easier to maintain when some external rules will be provided etc.

wtrocki · 2021-07-05T11:00:35Z

validator/rules/config.go

+		log.Fatal(err)
+	}
+
+	configHelper.Length.Verbose = defaultVerbose


What is that about?
Very bad smell in code. Direct assignment. Why not make rule working with the validationOptions?

validator/rules/config.go

wtrocki · 2021-07-05T11:01:51Z

validator/rules/executor.go

-func (config *RuleConfig) ExecuteRules(cmd *cobra.Command) []validator.ValidationError {
+// ExecuteRulesInternal executes all the rules
+// provided by ruleConfig
+func (ruleConfig *RuleConfig) ExecuteRulesInternal(cmd *cobra.Command, userValidatorConfig *ValidatorConfig) []validator.ValidationError {


Do you dissagree with what I have proposed?
In what I have proposed config should not have any method (not to mention internal)

@wtrocki addressed comments

wtrocki

This somewhat ended up being more convoluted than before.
Do you have any problems with what was proposed? Any challenges?

wtrocki · 2021-07-05T14:35:36Z

@ankithans would it be possible to update the documentation and example code?

wtrocki · 2021-07-05T14:36:30Z

validator/example/cmd_test.go

-	var vali rules.RuleConfig
-	validationErr := vali.ExecuteRules(cmd)
+	ruleCfg := rules.ValidatorConfig{
+		ValidatorOptions: rules.ValidatorOptions{Verbose: false},


Do we need to specify value that is used by default?

Suggested change

ValidatorOptions: rules.ValidatorOptions{Verbose: false},

@wtrocki review required

Of the whole PR? or just this?

ankithans added the bug Something isn't working label Jul 1, 2021

ankithans requested a review from wtrocki July 1, 2021 10:35

ankithans self-assigned this Jul 1, 2021

ankithans force-pushed the ankithans/issue104 branch from 5aa30a7 to 4e5ba58 Compare July 1, 2021 10:38

wtrocki reviewed Jul 1, 2021

View reviewed changes

dimakis reviewed Jul 2, 2021

View reviewed changes

ankithans linked an issue Jul 2, 2021 that may be closed by this pull request

Refactor RuleConfig to accept slice of RuleInterface #110

Closed

ankithans changed the title ~~fix: rule config overriding~~ refactor RuleConfig + overriding default values Jul 2, 2021

ankithans mentioned this pull request Jul 2, 2021

Refactor RuleConfig to accept slice of RuleInterface #110

Closed

ankithans force-pushed the ankithans/issue104 branch 3 times, most recently from 94cc90c to a0ffd91 Compare July 3, 2021 06:10

ankithans mentioned this pull request Jul 3, 2021

WIP Validator restructure #128

Closed

9 tasks

dimakis reviewed Jul 3, 2021

View reviewed changes

ankithans force-pushed the ankithans/issue104 branch from b3c76e2 to a94c0f1 Compare July 3, 2021 07:56

ankithans requested a review from wtrocki July 3, 2021 08:42

ankithans force-pushed the ankithans/issue104 branch from abdfc14 to 8ca36df Compare July 3, 2021 09:00

ankithans added 8 commits July 5, 2021 16:24

fix: rule config overriding

23e022a

refactor: rules as slice

4c61aef

fix: override length field

6e53b37

fix: mustExist rule overriding

4f466eb

chore: remove unused param in initDefaultRules

8a43148

refactor: separate user API from ruleConfig

5abd7a8

fix: check mergo, unmarshal for err

399d6c6

fix: global verbose & rename options

fbdaf5d

wtrocki reviewed Jul 5, 2021

View reviewed changes

validator/rules/config.go Show resolved Hide resolved

ankithans force-pushed the ankithans/issue104 branch from eaa082e to fbdaf5d Compare July 5, 2021 10:54

wtrocki reviewed Jul 5, 2021

View reviewed changes

validator/rules/config.go Show resolved Hide resolved

chore: run gofmt & provide docs

a5ed19c

wtrocki reviewed Jul 5, 2021

View reviewed changes

wtrocki suggested changes Jul 5, 2021

View reviewed changes

chore: address review comments

013122b

ankithans requested a review from wtrocki July 5, 2021 12:27

chore: pass golangci-lint

8891035

wtrocki reviewed Jul 5, 2021

View reviewed changes

docs: update validator docs

4635e1f

ankithans linked an issue Jul 5, 2021 that may be closed by this pull request

Fix lint errors generated by golangci-lint #133

Closed

ankithans requested a review from wtrocki July 5, 2021 15:58

wtrocki approved these changes Jul 5, 2021

View reviewed changes

wtrocki merged commit b2c727b into main Jul 5, 2021

wtrocki deleted the ankithans/issue104 branch July 5, 2021 16:09

ankithans mentioned this pull request Jul 5, 2021

Configure validation with a config file #141

Closed

refactor RuleConfig + overriding default values #105

refactor RuleConfig + overriding default values #105

Conversation

ankithans commented Jul 1, 2021 • edited

Description

Type of change

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

ankithans Jul 2, 2021 • edited

Choose a reason for hiding this comment

dimakis Jul 2, 2021 • edited

Choose a reason for hiding this comment

dimakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Jul 2, 2021

ankithans commented Jul 3, 2021

ankithans commented Jul 3, 2021

Merge this

into this

Result should be

dimakis commented Jul 3, 2021

ankithans commented Jul 3, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankithans Jul 3, 2021 • edited

Choose a reason for hiding this comment

ankithans Jul 3, 2021 • edited

Choose a reason for hiding this comment

ankithans commented Jul 3, 2021

wtrocki commented Jul 3, 2021

wtrocki commented Jul 3, 2021

ankithans commented Jul 3, 2021 • edited

wtrocki commented Jul 3, 2021

ankithans commented Jul 3, 2021

wtrocki commented Jul 3, 2021 • edited

ankithans commented Jul 3, 2021 • edited

wtrocki commented Jul 5, 2021

Choose a reason for hiding this comment

wtrocki Jul 5, 2021 • 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

wtrocki left a comment

Choose a reason for hiding this comment

wtrocki commented Jul 5, 2021

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

Choose a reason for hiding this comment

ankithans commented Jul 1, 2021 •

edited

ankithans Jul 2, 2021 •

edited

dimakis Jul 2, 2021 •

edited

ankithans commented Jul 3, 2021 •

edited

ankithans Jul 3, 2021 •

edited

ankithans Jul 3, 2021 •

edited

ankithans commented Jul 3, 2021 •

edited

wtrocki commented Jul 3, 2021 •

edited

ankithans commented Jul 3, 2021 •

edited

wtrocki Jul 5, 2021 •

edited