[persistent collections] based on PR-866 #1261
Conversation
| return fmt.Errorf("initcol: collection %s is not valid", col) | ||
| } | ||
| // we validate if this is a persistent collection | ||
| persistent := []string{"USER", "SESSION", "IP", "RESOURCE", "GLOBAL"} |
There was a problem hiding this comment.
Do we really need to restrict arbitrary collection creation?
| @@ -76,8 +78,10 @@ func (a *setvarFn) Init(_ plugintypes.RuleMetadata, data string) error { | |||
| colKey, colVal, colOk := strings.Cut(key, ".") | |||
| // Right not it only makes sense to allow setting TX | |||
| // key is also required | |||
| if strings.ToUpper(colKey) != "TX" { | |||
| return errors.New("invalid arguments, expected collection TX") | |||
| available := []string{"TX", "USER", "GLOBAL", "RESOURCE", "SESSION", "IP"} | |||
There was a problem hiding this comment.
@jptosso Clould you help me here? It's your changes, I believe you have more context here.
I personally see the reason of making constraints, just to minimize unpredictable behavior.
And in the future, if we really need we can extend it without breaking compatibility.
On the other hand if we make it possible to pass anything now and bring constrains after, there is a chance to break code for someone.
There was a problem hiding this comment.
Hey! setvar can only be used for this set of variables. Others are not mutuable, but IMOwe can use type assertion for this
| ) | ||
|
|
||
| // defaultEngine | ||
| // defaultEngine is just a sample and it shouldn't be used in production. |
There was a problem hiding this comment.
Shouldn't we use the third-party library then?
There was a problem hiding this comment.
We can use own custom engine if we need.
Take a look here.
| switch v := res.(type) { | ||
| case string: | ||
| return v, nil | ||
| case int: |
There was a problem hiding this comment.
Why do we even store it as int if we only set a value of type string?
| @@ -0,0 +1,50 @@ | |||
| // Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors | |||
There was a problem hiding this comment.
| // Copyright 2023 Juan Pablo Tosso and the OWASP Coraza contributors | |
| // Copyright 2024 Juan Pablo Tosso and the OWASP Coraza contributors |
Maybe even 2025.
There was a problem hiding this comment.
Not sure. These changes made in 2023 by Juan.
I'd keep it as it is.
|
Great job here. There is a PR for the close implementation #1200 |
|
JFYI: the PR is ready from my side. There are unit-tests and I checked it on my local machine with some rules. |
| // // SetOne will replace the key's value with this string | ||
| // SetOne(key string, value string) | ||
|
|
||
| // SetTTL will set the TTL for the key |
There was a problem hiding this comment.
There was a problem hiding this comment.
All the exported methods should have documentation.
There was a problem hiding this comment.
It's not necessary here.
It's not a part of the library but an example for users that they can implement their own pesistence engine and the proof that it works.
Look at the path
This PR is based on the PR-866 of the original repository.
It is related to the issue-1227.
Persistence collections
Thanks for your contribution ❤️