diff --git a/docs/adr/0006-where-to-implemenent-model-serialization.md b/docs/adr/0006-where-to-implemenent-model-serialization.md index 3ea9ab48a1..82caf08206 100644 --- a/docs/adr/0006-where-to-implemenent-model-serialization.md +++ b/docs/adr/0006-where-to-implemenent-model-serialization.md @@ -1,101 +1,92 @@ -# *TODO: Make decision* +# Separate metadata serialization from metadata class model but keep helpers -Technical Story: https://github.com/theupdateframework/tuf/pull/1223 -in particular [this PR discussion](https://github.com/theupdateframework/tuf/pull/1223#issuecomment-737188686). +Technical Story: https://github.com/theupdateframework/tuf/pull/1279 ## Context and Problem Statement In the course of implementing a class-based role metadata model we have also -reviewed options on how to design de/serialization infrastructure between wire +reviewed options on how to design serialization infrastructure between wire formats and the class model. In an initial attempt we have implemented -de/serialization on the metadata class (see considered option 1), but issues -with inheritance and calls for more flexibility have caused us to rethink this -approach. +serialization on the metadata class (see option 1), but issues with inheritance +and calls for more flexibility have caused us to rethink this approach. ## Decision Drivers -* A class-based role metadata model (see ADR 0004) requires de/serialization - routines from and to wire formats -* TUF integrators may choose custom de/serialization implementations to support +* A class-based role metadata model (see ADR4) requires serialization routines + from and to wire formats +* TUF integrators may choose custom serialization implementations to support custom wire formats -* Readability and simplicity of implementation +* Readability and simplicity of implementation for users and maintainers * Recognizability of specification ## Considered Options -1. De/serialization on metadata classes -2. De/serialization on metadata subclasses -3. De/serialization separated -4. Compromise 1: Default json de/serialization on metadata classes, - non-default de/serialization separated -5. Compromise 2: Dictitionary conversion for default json de/serialization on - metadata classes, general de/serialization separated - +1. Serialization on metadata classes +2. Serialization on metadata subclasses +3. Serialization separated +4. Compromise 1: Default json serialization on metadata classes, non-default + serialization separated +5. Compromise 2: Dictionary conversion required for default json serialization + on metadata classes, serialization generally separated ## Decision Outcome -*TODO: Make and describe the decision* +Chosen option: "Compromise 2", because it is idiomatic to implement dictionary +conversion as class methods and makes the code better structured, while +completely separating the serialization interface. ## Pros and Cons of the Options -### Option 1: De/serialization on metadata classes +### Option 1: Serialization on metadata classes -De/serialization is implemented as methods of the metadata classes, e.g. +Serialization is implemented as methods of the metadata classes, e.g. `Metadata.serialize_as_json`, etc. -* Good, because de/serialization for any object is encapsulated within the +* Good, because serialization for any object is encapsulated within the corresponding class and thus structured in small code chunks, using the already existing hierarchical class model structure. - -* Good, because we can use dedicated factory methods alongside constructors for - deserialization to cleanly separate deserialization from object instantiation. - * Good, because the TUF specification is heavily based on json (even if only - for illustrative purposes). Thus, providing the default json de/serialization - on the metadata classes facilitates recognizability. - + for illustrative purposes). Thus, this option facilitates recognizability. * Bad, because it might suggest that TUF is limited to json alone. - -* Bad, because the class model should be completely decoupled from the wire - format in order to facilitate the use of custom de/serialization - implementations. - +* Bad, because it does not facilitate custom serialization implementations. * Bad, because it can get complicated with inheritance in the class model - (NOTE: a workaround exist). + (NOTE: a workaround exist). -### Option 2: De/serialization on metadata subclasses -De/serialization is implemented as methods of corresponding metadata -subclasses, e.g. `JsonMetadata.serialize()`, `XmlMetadata.serialize()`, etc. +### Option 2: Serialization on metadata subclasses +Serialization is implemented as methods of corresponding metadata subclasses, +e.g. `JsonMetadata.serialize()`, `XmlMetadata.serialize()`, etc. -* Good, because the wire format de/serialization is decoupled from base classes. -* Bad, because a user needs to decide on de/serialization ahead of time, when - instantiating respective de/serialization metadata objects. +* Good, because the wire format serialization is decoupled from base classes, + not giving the impression that TUF is limited to json, and facilitating + custom implementations. +* Bad, because a user needs to decide on serialization ahead of time, when + instantiating respective serialization metadata objects. * Bad, because the metadata model has many classes, which would all need to be subclassed accordingly. -### Option 3: De/serialization separated -De/serialization is implemented independently of the metadata class, by -defining an abstract `Serializer` interface with de/serialization methods, +### Option 3: Serialization separated +Serialization is implemented independently of the metadata class, by +defining an abstract `Serializer` interface with serialization methods, which concrete implementations, e.g. `JsonSerializer`, `XmlSerializer`, etc., must subclass and override. -* Good, because wire format de/serialization is completely decoupled from class - model, thus repository or client code can more easily account for custom - de/serialization implementations. - -* Bad, because a decoupled de/serialization implementation needs to - "re-implement" the entire class hierarchy, likely in a procedural manner. +* Good, because wire format serialization is completely decoupled from class + model, not giving the impression that TUF is limited to json, and facilitating + custom implementations. +* Good, because it can serve as exact blueprint for custom implementations. +* Bad, because a decoupled serialization implementation needs to "re-implement" + the entire class hierarchy, likely in a procedural manner. ### Option 4: Compromise 1 -Default json de/serialization is implemented on the metadata class as described +Default json serialization is implemented on the metadata class as described in (1), with the option to override it using concrete subclass implementations of a separate `Serializer` interface as described in (3). * Good, for the reasons outlined in options (1) and (3). -* Bad, because it creates two completely different code paths for default and - non-default formats making the code more complex and thus harder to maintain, - and also prone to deteriorate, especially on the non-default path. +* Bad, because it creates two different code paths for default and non-default + formats making the code more complex and thus harder to maintain, and also + prone to deteriorate, especially on the non-default path. * Bad, because the on-the-class default implementation can not be used as - blueprint for separate non-default implementations. + blueprint for custom implementations. ### Option 5: Compromise 2 -De/serialization is implemented independently of the metadata class as +Serialization is implemented independently of the metadata class as described in (3), however, the *meat* of the default `JsonSerializer`, i.e. conversion between metadata objects and dictionaries, is implemented on the metadata class, e.g. as `Metadata.to_dict`, etc. @@ -103,16 +94,18 @@ metadata class, e.g. as `Metadata.to_dict`, etc. * Good, for the reasons outlined in options (1) and (3) without the disadvantage in (4) of having two completely different code paths. * Good, because conversion between class objects and dictionaries is akin to - type casting (not serialization), which feels idiomatic to implement on the - class. + type casting, which is idiomatic to implement on the class. * Good, because it keeps the separate default serializer minimal (only a thin json wrapper around `to_dict`). - +* Bad, because the on-the-class default implementation can not be used as + blueprint for separate non-default implementations. ## Links -* [ADR 0004: Add classes for complex metadata attributes (decision driver)](/Users/lukp/tuf/tuf/docs/adr/0004-extent-of-OOP-in-metadata-model.md) +* [ADR4: Add classes for complex metadata attributes (decision driver)](/Users/lukp/tuf/tuf/docs/adr/0004-extent-of-OOP-in-metadata-model.md) * [PR: Add simple TUF role metadata model (option 1)](https://github.com/theupdateframework/tuf/pull/1112) - * [details about separation of de/serialization and instantiation](https://github.com/theupdateframework/tuf/commit/f63dce6dddb9cfbf8986141340c6fac00a36d46e) - * [code comment about issues with inheritance](https://github.com/theupdateframework/tuf/blob/9401059101b08a18abc5e3be4d60e18670693f62/tuf/api/metadata.py#L297-L306) + - [details about separation of serialization and instantiation](https://github.com/theupdateframework/tuf/commit/f63dce6dddb9cfbf8986141340c6fac00a36d46e) + - [code comment about issues with inheritance](https://github.com/theupdateframework/tuf/blob/9401059101b08a18abc5e3be4d60e18670693f62/tuf/api/metadata.py#L297-L306) +* [PR: New metadata API: add MetadataInfo and TargetFile classes (recent ADR discussion impetus)](https://github.com/theupdateframework/tuf/pull/1223) + - [discussion about inheritance issues in option 1](https://github.com/theupdateframework/tuf/pull/1223#issuecomment-737188686) * [SSLIB/Issue: Add metadata container classes (comparison of options 1 and 2)](https://github.com/secure-systems-lab/securesystemslib/issues/272) * [tuf-on-a-plane parser (option 3)](https://github.com/trishankatdatadog/tuf-on-a-plane/blob/master/src/tuf_on_a_plane/parsers/)