konform-kt / konform

Portable validations for Kotlin
https://www.konform.io
MIT License
651 stars 39 forks source link

`onEach` working for nullable iterables #33

Closed wtomi closed 2 years ago

wtomi commented 2 years ago

Currently, one cannot apply onEach validation if iterable field is nullable, for example val items: List<MyClass>? There should be some way to do it either by making it possible to combine ifPresent with onEach or by making onEach treat a null iterable in the same way as an empty iterable. There could also be new method like onEachIfPresent

nlochschmidt commented 2 years ago

This is an interesting issue you encountered. Thanks for raising it.

Let's flesh out the options a bit more.

Given

data class Data(val registrations: List<Register>?)

Solution from provided MR

The option enabled by #34 would transparently enable onEach to work on nullable lists like this:

Data::registrations onEach {
    Register::email {
        minLength(3)
    }
}

I am not sure I like this as it identical with

Data::registrations ifPresent {}
Data::registrations onEach {
    Register::email {
        minLength(3)
    }
}

but different to

Data::registrations required {}
Data::registrations onEach {
    Register::email {
        minLength(3)
    }
}

The alternatives you are proposing are:

Option: New method

Data::registrations onEachPresent {
      Register::email {
          minLength(3)
      }
 }
Data::registrations onEachRequired {
    Register::email {
        minLength(3)
    }
 }

Option: Nested

Data::registrations ifPresent {
    onEach {
        Register::email {
            minLength(3)
        }
     }
 }
Data::registrations required {
    onEach {
        Register::email {
            minLength(3)
        }
     }
 }

I think I actually like the last option with nested onEach best, because it is the only one that would allow us to have the constraints for the iterables together e.g. as in this example:

Data::registrations ifPresent {
    minItems(1)
    maxItems(10)
    onEach {
        Register::email {
            minLength(3)
        }
    }
}

What do you think?

wtomi commented 2 years ago

I also like the nested option. It seems like a natural extension because onEach would work in the same way as it has been. I can try to implement it later.

matfiej88 commented 2 years ago

+1 for nested option

nlochschmidt commented 2 years ago

@wtomi I finally came around to trying this and based it on the work you already did in #34

nlochschmidt commented 2 years ago

This will become part of the 0.4 release.

wtomi commented 2 years ago

It's a great news. Thank you!