Raise ValidationError for unhashable discriminator values

[kurtmckee]

As reported in #4773, unhashable discriminator values are incorrectly rejected with a ConfigError, rather than a ValidationError.

The bug

The current code catches a TypeError at line 1121 and converts it to a ConfigError:

pydantic/pydantic/fields.py

Lines 1120 to 1127 in 7f3b754

	try:
	sub_field = self.sub_fields_mapping[discriminator_value] # type: ignore[index]
	except TypeError:
	assert cls is not None
	raise ConfigError(
	f'field "{self.name}" not yet prepared so type is still a ForwardRef, '
	f'you might need to call {cls.__name__}.update_forward_refs().'
	)

This can happen when self.sub_fields_mapping is None, in which case the ConfigError correctly suggests calling .update_forward_refs(). For reference, the exact error is:

None[discriminator_value]  # TypeError: 'NoneType' object is not subscriptable

However, a TypeError can also be raised when self.sub_fields_mapping is a dictionary but discriminator_value is an unhashable type. For reference, the exact error is:

{"valid": "dict"}[{}]  # TypeError: unhashable type: 'dict'

The fix

This PR addresses the bug by first checking whether self.sub_fields_mapping is None. If it is, then a ConfigError is raised as expected. Then, both KeyError and TypeError are interpreted as an invalid discriminator value.

Please let me know if this needs to be separately forward-ported to main.

Thanks for your work on pydantic!

Fixes #4773

[kurtmckee]

The GitHub workflow failed to start for this. If it's not possible to re-run the failed jobs, let me know and I'll push an update to re-trigger the workflow.

[hramezani]

Thanks @kurtmckee for this.

The fix LGTM.

The only concern is that it is a change in old behavior(raising ConfigError) and I am afraid users depend on this(catching ConfigError on their code base).

Please let me know if this needs to be separately forward-ported to main.

I think it's not needed.

@samuelcolvin, @dmontagu what do you think?

[dmontagu]

@hramezani

The only concern is that it is a change in old behavior(raising ConfigError) and I am afraid users depend on this(catching ConfigError on their code base).

I think this is a very good point. I'm definitely keen to make sure this doesn't happen in v2, but I'd be a bit nervous to change the error handling in older versions unless this was causing issues that couldn't be properly handled by catching a ConfigError in the right place.

[hramezani]

@dmontagu

but I'd be a bit nervous to change the error handling in older versions unless this was causing issues that couldn't be properly handled by catching a ConfigError in the right place.

Agree!

[kurtmckee]

It's my understanding that ConfigError would indicate a problem with the users' models, which is not the case here.

This appears to be a ValidationError, which is what users would anticipate having to catch when the discriminator key is invalid. I think that fixing this aligns the behavior with user expectations and code, and helps prevent production code from breaking due to an incorrect exception.

[dmontagu]

@kurtmckee I agree that what you are describing is better behavior, the main point is that if someone is already handling it as a ConfigError, it could end up breaking their code if they upgrade. If we were still planning additional releases to pydantic v1 this would definitely make sense as an addition to a 1.11.0 release, but we aren't planning to do that now (instead, doing a 2.0.0).

That said, I recognize that this is arguably a bugfix worthy of inclusion in a 1.10.X release; I'll defer to @samuelcolvin's opinion on what's the best path forward.

[dmontagu]

Thought about this more and I think you are right about everything @kurtmckee. Going to merge this then do a new release.

[kurtmckee]

Thank you @dmontagu, and thank you all for reviewing and discussing!