Fix incorrect bitmask exposed by DeferredMemberSchema #842
Conversation
| var target = memberTarget(); | ||
| return SchemaBuilder.computeRequiredBitField( | ||
| type(), | ||
| target.requiredMemberCount(), | ||
| target.members(), | ||
| Schema::requiredByValidationBitmask); |
There was a problem hiding this comment.
I'm not sure this change is really needed .. I haven't been able to reproduce any bad behavior without it. This new code matches MemberSchema's behavior (but lazy/deferred). On the other hand, it seems like MemberSchema could just delegate to target.requiredStructureMemberBitfield() like this used to, and still be correct as long as the requireStructureMemberBitfield value cannot change for a given schema once build. Maybe that's a better change, simpler overall?
There was a problem hiding this comment.
Yeah I am inclined for the latter. I don't think the requireStructureMemberBitfield changes once built.
There was a problem hiding this comment.
Thanks Adwait -- this makes the most sense to me too. Updated.
| @@ -26,7 +26,7 @@ final class MemberSchema extends Schema { | |||
| this.requiredStructureMemberBitfield = SchemaBuilder.computeRequiredBitField( | |||
| type(), | |||
| target.requiredMemberCount(), | |||
| builder.target.members(), | |||
| target.members(), | |||
There was a problem hiding this comment.
Incidental change for consistency with line 28 -- target and builder.target are the same at this point.
The bitmask applies to the member, not its target schema -- delegating to the target schema exposes the wrong bitmask. Also change MemberSchema.requiredStructureMemberBitfield() to delegate to its target, because the bitfield *is* derived from the target type (not the specific member usage of the target type).
rationull
force-pushed
the
deferred-member-schema-bitmask
branch
from
d75f15f to
1d509ab
Compare
Issue #, if available:
I did not file an issue.
Description of changes:
The bitmask applies to the member, not its target schema -- delegating to the target schema exposes the wrong bitmask.
Also fix an incidental similar issue in
requiredStructureMemberBitField(), though I haven't seen any negative effects from the previous implementation of that one.
This is a pretty esoteric issue, reproduction with Validator requires a DeferredMemberSchema to exist with the right characteristics, and not be replaced with a concretely built MemberSchema prior to being operated on. I identified this case when debugging a member schema incorrectly marked as recursive due to #841. Esoteric or not, the existing behavior seems wrong to me; happy to be corrected if I'm missing something.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.