Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reject extra keys on deserialization #197

Closed
real-or-random opened this issue Mar 13, 2024 · 7 comments · Fixed by #198
Closed

Reject extra keys on deserialization #197

real-or-random opened this issue Mar 13, 2024 · 7 comments · Fixed by #198
Labels
enhancement New feature or request

Comments

@real-or-random
Copy link

Is your feature request related to a problem? Please describe.
When deserializing, it can happen that the input (say a dict) contains extra/unknown keys, e.g., due to a typo in a configuration file, or due to a mismatch in the data model.

Describe the solution you'd like
It would be great to have an option to reject extra keys during deserialization, similar to https://catt.rs/en/stable/customizing.html#forbid-extra-keys in cattr.

I was surprised that this has not been asked before, it seems a very natural thing to me. At least I couldn't find anything/ (It's very vaguely related to #42 .)

Describe alternatives you've considered
I can't think of a good alternative that works well with this library. I could manually check that all keys are known, but doing this manually defeats the purpose of using a library for deserialization.

@Fatal1ty
Copy link
Owner

Fatal1ty commented Mar 13, 2024

I was surprised that this has not been asked before, it seems a very natural thing to me.

In my experience ignoring extra keys is a blessing in scenarios when you want to be forward compatible. Maybe, I’m not the only one in this boat, so this may explain why this feature hasn’t been requested yet :)

Actually, the first one who wanted to forbid extra keys was @mishamsk, and we are waiting for him to split his valuable pull request #183 (comment) that will probably resolve this issue.

Meanwhile, I have a list of tasks that need to be solved by the next release (and #42 is one of them). However, if this is a blocker for you, I might consider implementing it myself or taking the PR from @mishamsk as a basis if he doesn’t mind. At least we now have this feature request documented :)

@Fatal1ty Fatal1ty added the enhancement New feature or request label Mar 13, 2024
@mishamsk
Copy link
Contributor

hey all. I wrote a long comment why I won't be able to split that until April and then it got boring and I just split the code. Hopefully now you'll be ok to merge one or both of the PRs.

P.S. I'd say a just/taskfile/make with a command to run the preferred linting that would match CI test would be cool!

@Fatal1ty
Copy link
Owner

@mishamsk Welcome back! I will look into this as soon as possible, most likely this week.

P.S. I'd say a just/taskfile/make with a command to run the preferred linting that would match CI test would be cool!

This is a good point. I personally got used to run all linters separately on my laptop but this needs to be automated. I’ll handle this.

@mishamsk
Copy link
Contributor

This is a good point. I personally got used to run all linters separately on my laptop but this needs to be automated. I’ll handle this.

I've recently started using taskfile (e.g. here is one of them - most of the cleanup is via pre-commit which is also called from GitHub action). Taskfile is pretty nice and easy to write tasks (I have much more complicated stuff in private repos), although one think I miss that just has - ability to pass arbitrary args without -- .... So both options seems great, especially compared with Makefile...

@real-or-random
Copy link
Author

@mishamsk Nice to see a PR. :) I had missed your (now closed) PR because I only searched through issues...

In my experience ignoring extra keys is a blessing in scenarios when you want to be forward compatible.

This made me think. I assume one common thing that applications may want to do is ignore but not silently, i.e., issue a warning to the user or write into the log file.

In fact, that's true for other errors, too. One general approach is to ignore errors (e.g., skip invalid values and extra keys) but record them into a list of exceptions and finish parsing to the end. Then at the end, if they were errors, throw an exception and include the parsed object in the exception, and the list of other exceptions. This does the right thing in the sense that it raises a real error if the user asks for it, but the caller can still continue. But the approach is pretty heavy, possibly too much for this kind of library (and certainly not to be included in the current PR).

Sorry for throwing so many suggestions at you... ^^ These are really rather just my ideas, not things that I'd need.

@Fatal1ty
Copy link
Owner

Fatal1ty commented Mar 17, 2024

P.S. I'd say a just/taskfile/make with a command to run the preferred linting that would match CI test would be cool!

On your request https://github.com/Fatal1ty/mashumaro/blob/master/justfile

P.S. Sadly, it seems like zsh completions work only for the first recipe. When I type the first letter of the second recipe, I don't see any suggestions on ⇥ Tab.

@mishamsk
Copy link
Contributor

P.S. I'd say a just/taskfile/make with a command to run the preferred linting that would match CI test would be cool!

On your request https://github.com/Fatal1ty/mashumaro/blob/master/justfile

P.S. Sadly, it seems like zsh completions work only for the first recipe. When I type the first letter of the second recipe, I don't see any suggestions on ⇥ Tab.

awesome. completions or not!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants