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

test: failing test for flattened nullable value object #58

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jameslkingsley
Copy link

This PR adds a failing test for a scenario that doesn't seem to be covered and I'm currently having issues with. I've had a crack at fixing it but I can't figure it out.

Quite a common pattern in my code is a nullable value object, where the parent is nullable but the value object itself contains a non-nullable value. You'll see an example of this in the test I've written.

I also make use of the flattening stuff to rename the value object to something more relevant while still retaining reusability of the value object itself.

Hopefully this helps maintainers here to identify the issue and point me in the right direction 🙏🏻

@jameslkingsley
Copy link
Author

Also to add to this, I noticed that in the tests the assertions happens between a Serde serialisation and deserialisation. This sort of makes sense but as you can see in this PR, doing that masks these kinds of issues because the serialisation is omitting the erroneous field.

tests/SerdeTestCases.php Outdated Show resolved Hide resolved
@Crell
Copy link
Owner

Crell commented Aug 2, 2024

Oh geez. This is tricky, because null just breaks bloody well everything. 😢 If I remove the guards that cause the field to be omitted, I get a cascading set of null errors in the code that follows.

@Crell
Copy link
Owner

Crell commented Aug 2, 2024

Well, I think this works on the serialization side. I hate it, but it seems to work. But now it also fails on the deserialization side, and I... have really no idea how to resolve that nicely.

@Crell
Copy link
Owner

Crell commented Aug 2, 2024

Here's the issue for deserialization (notes for me or someone else to pick up later): Flattening works by pulling out the values of an incoming stream that are relevant for that object, and then anything "remaining" gets passed to the collecting/flattening-marked properties with a "here, pull out what you want". That's what makes it flexible and recursive.

However, in this case, that means passing order_id to the child, which will dutifully try to read that as someone passing null to the id (aka value) property. Which will, correctly, fail with a type error when trying to create the object.

The problem is, we have no way to tell in advance that it's going to not work; It would require looking up the child object properties, looking to see if any are "not good" (by some definition), and then defaulting back to null. However, that is slow as it duplicates work, has no clear definition of "not good", and could possibly work only for single-value sub-objects.

At the moment, I think nullable flattened objects are just not supportable. I see no not-hideously-hardcoded way to do it.

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

Successfully merging this pull request may close these issues.

2 participants