fix(serialization): incorrect map serialization

[saibotk]

When switching to Symfony's serializer, the MapDenormalizer was implemented, but not added to the Serializer parameters. Thus, the attributes of a realm and other fields using the Map type were always empty.

Additionally, the MapNormalizerTest contained test data for the denormalizer. We adjusted this to correctly ingest Map's as the test values.

To properly normalize the Map type, we made the inner array publicly accessible. Previously the ArrayObject would otherwise hold an array with the entire Map object wrapped by the Map classname. This was addressed, by directly accessing the inner array and passing it to the ArrayObject.

We also added an integration test to prevent these kind of issues in the future, and to test the update method for realm attributes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.20%. Comparing base (2e9fb00) to head (1530c84).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #139   +/-   ##
=======================================
  Coverage   99.20%   99.20%           
=======================================
  Files          26       26           
  Lines         756      756           
=======================================
  Hits          750      750           
  Misses          6        6           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fschmtt]

Thanks a lot for your PR 🙏

Looks good to me, the only thing I'm not exactly sure is whether we should expose the $map attribute or use e.g. \ArrayAccess instead 🤔

[saibotk]

Always happy to give something back.

Well to be honest id prefer to remove the map type entirely. But i don't know that much about the usage so you can judge that with more background.

I don't think there is any issue with it as it is also marked readonly and already accessible via the json method just converted to an object.

Currently it is also rather hard DX wise to alter attributes, because they are contained in the Map. And we would need to access the array to create a new Map with the existing entries of an existing Map to alter the content.
(When not using the array iterator that is)

But im open for suggestions and your input here.

[fschmtt]

I think the reason for the map to exist is because I wanted to stay as close to the types used in Keycloak itself - a Map as it exists in Java doesn't exist as such in PHP, so I created this simple "wrapper" for an array.

I also intended to go the "objects are immutable" way, but I see how this degrades DX for using maps in this library 😅 I didn't quite make up my mind right now about the Map type, but maybe I can find a way like with the representations to easily modify its items.

Do you mind if we for now just fix the missing MapDenormalizer and find a solution to improving the Map type separately? Fixing the bug seems quite urgent to me, while for the maps I think I need some more time to try out different approaches 😇

Thanks for the discussion, I very much appreciate it!

[saibotk]

Ah sure we can discuss this first.

I need to verify again tomorrow if this works without the access to the array but i think it was necessary to do so because the serialization result had the inner array wrapped behind the class name as a key when throwing in the map type into the serializer.

Ill get back to you tomorrow 🙌

[saibotk]

Okay, we somehow need access to the underlying map array, otherwise as i said, the serialization will encapsulate the data wrapped in the "map" attribute:

See this test output for example:

Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
 ArrayObject Object (
-    'a' => 1
-    'b' => 2
-    'c' => 3
+    'map' => [...]
 )

In the next iteration, we also should consider not tagging the Map as @internal, as it has to be used to modify attributes etc.

[fschmtt]

Thank you for all your efforts with this issue!

I'm super hesitant to break the consistency with the rest of the code by changing the $map property to public and making it writable directly 🙈

Looking at the MapNormalizerTest IDK what I did there TBH - actually the normalizer is only ever called with a Map object, so the data provider does not really make sense. This should be ensured by the Symfony Serializer component.

WDYT about adding the necessary modifier methods to the Map type similar to the Representation instead of making the $map property public? Thought of something like e.g.:

    // other methods like e.g. ::has(string $key) or ::get(string $key)
    // as well as some ::toArray() maybe

    public function with(string $key, $value): self
    {
        $clone = clone $this;

        $clone->map[$key] = $value;

        return $clone;
    }

    public function without(string $key): self
    {
        $clone = clone $this;

        unset($clone->map[$key]);

        return $clone;
    }

This keeps the immutability aspect and follows the pattern of how other parts of the library are designed and implemented. I'll create new branch off of yours trying to implement these bits 😄

[saibotk]

Oh sure it is your project, I'm just giving you my opinion here and i can understand why you do not want to do this.

Though i think this whole thing feels like following an ideology just for the sake of consistency / alignment with Keycloak's Java code.
A lighter more PHP (-Developer) friendly approach might be nice for future iterations. Currently the handling of the Map and Representations feels rather unnatural and a bit bloated code-wise, without any benefit. I don't intent to badmouth your work here, just trying to provide another opinion on the design. For example the official JS implementation also is rather slim.

For now, i think your suggested improvements are a good solution for the current issue and I'd love to discuss future ideas at any time :)

Coming back to this PR though:

What is your proposed solution to the Map type that i should go for in this PR to make it merge-able?

Thanks for your timely answers its great to discuss such changes!

[fschmtt]

With consistency I was referring to how the representations are implemented because their properties are also encapsulated and they are immutable 😄It would break consistency with the existing DX, not with Java's types.

If PHP would allow some kind of generics, the Map wouldn't be necessary in the first place. Having the Map makes static analysis a lot easier and helps a little with "type-safety", though. I wouldn't mind getting rid of it, but right now there's no adequate alternative IMO 🫤

I've picked your commit and made adjustments in #140. If the changes to the Map in this commit suit your needs I'm happy to merge 😊

[saibotk]

Sounds reasonable and thanks for maintaining this library!

The changes in your PR look good to me!

Thanks again 🫶

[fschmtt]

Thank you too! I merged and released v0.28.0 🎉

It didn't recognize your commit was included, but I mentioned you in the release notes 😊