fixup! (wip) Have different types for file, HTML, and external attach…

…ments

rfc-116-store-attachment-data-in-content-items.md

@@ -68,15 +68,15 @@ shouldn't conflate the two.
 
 I propose we do this:
 
-1. Split `asset_link` into three new types: `attachment_asset`,
+1. Split `asset_link` into three new types: `file_attachment_asset`,
    `specialist_publisher_attachment_asset` and `image_asset`, updating
    the schemas which use `asset_link` accordingly, with these fields:
 
-   - `attachment_asset`:
+   - `file_attachment_asset`:
      - `url` (required)
      - `content_type` (required)
      - `title`
-   - `specialist_publisher_attachment_asset` (extends `attachment_asset`):
+   - `specialist_publisher_attachment_asset` (extends `file_attachment_asset`):
      - `content_id` (required)
      - `created_at`
      - `updated_at`
@@ -91,9 +91,10 @@ I propose we do this:
 
 1. Add new optional fields for extra metadata:
 
-   - `attachment_asset`:
+   - `file_attachment_asset`:
      - `accessible`
      - `alternative_format_contact_email`
+     - `attachment_type` (enum with permitted values `"file"`)
      - `file_size`
      - `filename`
      - `id`
@@ -108,10 +109,27 @@ I propose we do this:
    **Migration concerns:** This can be done without breaking
    compatibility, as all new fields are optional.
 
-1. Add new attachment type for publication attachments:
+1. Add new attachment types for HTML and external attachments:
 
-    `publication_attachment_asset` (extends `attachment_asset`) with fields:
-    - `attachment_type` (required)
+   - `html_attachment_asset`:
+     - `attachment_type` (required; enum with permitted values `"external"`)
+     - `id`
+     - `locale`
+     - `title`
+     - `url` (required)
+   - `external_attachment_asset`:
+     - `attachment_type` (required; enum with permitted values `"external"`)
+     - `id`
+     - `locale`
+     - `title`
+     - `url` (required)
+
+   **Migration concerns:** This can be done without breaking
+   compatibility, as the new types are unused for now.
+
+1. Add new attachment types for publication attachments:
+
+    `publication_attachment_asset` (one-of combination of `file_attachment_asset`, `html_attachment_asset`, and `external_attachment_asset`):
     - `command_paper_number`
     - `hoc_paper_number`
     - `isbn`
@@ -137,11 +155,11 @@ I propose we do this:
    **Migration concerns:** This can be done without breaking
    compatibility, as the new type will be unused for now.
 
-1. Make `id` in `attachment_asset` mandatory.
+1. Make `attachment_type` and `id` in `file_attachment_asset` mandatory.
 
     **Migration concerns:** this would need to be done in three steps:
     1. Update specialist publisher, travel advice publisher, and
-       manuals publisher to set an arbitrary ID for attachments.
+       manuals publisher to set the fields.
     2. Republish all affected documents.
     3. Make the field mandatory.
 
@@ -294,7 +312,7 @@ The final schemas will be:
 I've validated this proposed schema by regenerating the full schemas
 and checking the examples after making the following changes:
 
-- Change `travel_advice` to use `attachment_asset` and `image_asset`
+- Change `travel_advice` to use `file_attachment_asset` and `image_asset`
 - Change `asset_link_list` to use `specialist_publisher_asset`
 
 The `what-is-content-design.json` example failed as it is missing
@@ -318,6 +336,12 @@ attachments, so there is one list with everything in.  The
 providing the ordered list of document-level attachments for
 publication-like document types.
 
+**Why make the lists mandatory?**
+
+Is there a difference between a missing list and a present, but empty,
+list?  Maybe not, but I think it's better to be explicit that there
+are no attachments for a document.
+
 **Why split `asset_link`?**
 
 `asset_link` is currently used for two different things: images and
@@ -329,8 +353,19 @@ for example.  These should be separate types.
 
 Different publishing apps have different allowable metadata, so it
 makes sense to have different types constraining what a publishing app
-is able to provide.  However, we still want some unifying structure
-behind the types, so we use inheritance.
+is able to provide.
+
+We have prior art in using a single-valued enum to make a tagged
+union: it's how the top-level schemas do it, eg:
+
+```json
+"schema_name": {
+  "type": "string",
+  "enum": [
+    "detailed_guide"
+  ]
+},
+```
 
 **Why `publication_attachment_asset`?**
 
@@ -350,12 +385,6 @@ Manuals Publisher and Travel Advice Publisher have their own
 databases, rather than using Publishing API directly as a backing
 store, so they dont have this same problem.
 
-**Why make the lists mandatory?**
-
-Is there a difference between a missing list and a present, but empty,
-list?  Maybe not, but I think it's better to be explicit that there
-are no attachments for a document.
-
 [schema.org]: http://schema.org/
 [see this comment]: https://github.com/alphagov/govuk_publishing_components/pull/1247#pullrequestreview-338008254
 [`asset_link` type]: https://github.com/alphagov/govuk-content-schemas/blob/47a751e7eb193738c2ec43be03b149527a2b8e15/formats/shared/definitions/asset_links.jsonnet