-
Notifications
You must be signed in to change notification settings - Fork 275
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
ADR0006: Where to implement model serialization (WIP)
Finalize ADR0006 (wordsmith, shorten, address TODO's, ...) TODO: - Proof read - squash commits Signed-off-by: Lukas Puehringer <[email protected]>
- Loading branch information
Showing
1 changed file
with
56 additions
and
63 deletions.
There are no files selected for viewing
119 changes: 56 additions & 63 deletions
119
docs/adr/0006-where-to-implemenent-model-serialization.md
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,118 +1,111 @@ | ||
| # *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. | ||
|
|
||
| * 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/) |