yassun7010 / serde_valid

JSON Schema based validation tool using serde.
MIT License
46 stars 10 forks source link

Add recursive implementation of validation to HashMap, leaving max/min properties unchanged #9

Closed nickjknight closed 1 year ago

nickjknight commented 1 year ago

This solves issue #7 that I raised: https://github.com/yassun4dev/serde_valid/issues/7

Fundamentally, I applied a modified version of the compositedTrait impl of Vec to Hashmaps, checking the values inside the HashMap.

I add a new macro invocation for creating the impls that is only called on the string validations that allows the values in a hashMap to be validated against. This keeps the impls of validate on hashmap for max/min properties unchanged as checking len is clearly still what is wanted there.

I also add the same recursive Value validation of hashmaps to the rest of the impls that have - numbers etc. These were simple and simply worked.

Finally, I added a test for this new impl. Unfortunately for the error case there I couldn't just check the full json output due to the need to return the index from the compositedTrait impl and the fact that HashMaps are not ordered, so I check the errors separately instead, making sure that errors that appear multiple times appear the correct number of times.

nickjknight commented 1 year ago

I've left the version bump for you to do once this is merged. I hope that's Ok.

yassun7010 commented 1 year ago

@nickjknight

Thank you for your pull request.

Please change the error message type to validation::ObjectErrors instead of validation::ArrayErrors, and I will merge them.

nickjknight commented 1 year ago

I've changed that error type in lib.rs (I think that's all you were asking for). That did cause me to limit the implementation of HashMap there to Ks that at &'static str thanks to the objectErrors internal types. I think i've done the right thing, but I'm not totally clear on why you wanted the error type changing in the first place, so check that I've done what you would've.

yassun7010 commented 1 year ago

@nickjknight

Thanks for fixing it!

I will merge this in and release it after I take the time this weekend to improve it too.

I avoided validation::ArrayErrors because HashMap does not guarantee the order of keys, so error messages by index are confusing to users.

nickjknight commented 1 year ago

Thanks for your attention. Once v0.16 is released we can close issue 7

nickjknight commented 1 year ago

Ah shoot, I have realised that using &'static str as the hashmap impl for lib.rs validate does not actually work for my use-case. I'm going to open another merge request and try to solve this while still using the objectError instead of the arrayError.

nickjknight commented 1 year ago

Unfortunately, I spent some time on this but couldn't figure out a way of making it work with objectErrors. The issue is I want the key from the Hashmap, but that can't be a &'static str, and I don't have access to the serialised static str to put there anyway. If you can find a way of getting the hashmap key into that index map then that'd be great, but for now, I think this is the best thing that'll actually work (as shown by the added test): https://github.com/yassun4dev/serde_valid/pull/10