Make entity ids mandatory in Bulk update/patch requests (#1460)

Fix a bug in the Bulk update/patch routes by enforcing the ids in the document updates as mandatory in the request schemas

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="iroh-services-clients">[§](#iroh-services-clients)</a> IROH Services Clients
=====================================================================================

Put all informations that need to be communicated to IROH Services Clients.
Typically IROH-UI, ATS Integration, Orbital, etc...
 -->

<a name="qa">[§](#qa)</a> QA
============================


Describe the steps to test your PR.

1. issue a bulk update request without supplying ids in the entities
2. the request should fail with an appropriate 400 response
3. profit



<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="ops">[§](#ops)</a> Ops
===============================

  Specify Ops related issues and documentation
- Config change needed: threatgrid/tenzin#
- Migration needed: threatgrid/tenzin#
- How to enable/disable that feature: (ex remove service from `bootstrap.cfg`, add scope to org)
-->

<!-- UNCOMMENT THIS SECTION IF NEEDED
<a name="documentation">[§](#documentation)</a> Documentation
=============================================================

  Public Facing documentation section;
- Public documentation updated needed: threatgrid/iroh-ui#
  See internal [doc file](./services/iroh-auth/doc/public-doc.org)
 -->

<a name="release-notes">[§](#release-notes)</a> Release Notes
=============================================================

<!-- REMOVE UNUSED LINES -->

```
public: harden the bulk update schemas
```

<a name="squashed-commits">[§](#squashed-commits)</a> Squashed Commits
======================================================================

src/ctia/bulk/routes.clj

@@ -53,7 +53,7 @@
          :responses {200 {:schema (s/maybe (bulk.schemas/BulkActionsRefs services))}}
          :summary "UPDATE many entities at once"
          :query-params [{wait_for :- (describe s/Bool "wait for updated entities to be available for search") nil}]
-         :body [bulk (describe (bulk.schemas/BulkUpdate services) "a new Bulk Update object")]
+         :body [bulk (describe (bulk.schemas/BulkUpdate services) "a Bulk Update object")]
          :description (common/capabilities->description capabilities)
          :capabilities capabilities
          :auth-identity auth-identity

src/ctia/bulk/schemas.clj

@@ -25,26 +25,34 @@
             s/Keyword s/Any})])
 
 (defn entity-schema
-  [{:keys [plural] :as entity} sch services]
-  (let [bulk-schema
-        (if (keyword? sch)
-          [(s/maybe
-            (let [ent-schema (get entity sch)]
-              (if (fn? ent-schema) (ent-schema services) ent-schema)))]
-          sch)]
-    {(-> plural
-         name
-         (str/replace #"-" "_")
-         keyword)
-     bulk-schema}))
+  ([entity sch services]
+   (entity-schema entity sch services {}))
+  ([{:keys [plural] :as entity} sch services {:keys [required-ids?]}]
+   (let [bulk-schema
+         (if (keyword? sch)
+           [(s/maybe
+             (let [ent-schema (get entity sch)]
+               (cond-> (if (fn? ent-schema)
+                         (ent-schema services) ent-schema)
+                 required-ids? (st/required-keys [:id]))))]
+           (if required-ids?
+             (st/required-keys sch :id)
+             sch))]
+     {(-> plural
+          name
+          (str/replace #"-" "_")
+          keyword)
+      bulk-schema})))
 
 (defn entities-bulk-schema
-  [entities sch services]
-  (st/optional-keys
-   (->> (vals entities)
-        (remove :no-bulk?)
-        (map #(entity-schema % sch services))
-        (apply merge {}))))
+  ([entities sch services]
+   (entities-bulk-schema entities sch services {}))
+  ([entities sch services opts]
+   (st/optional-keys
+    (->> (vals entities)
+         (remove :no-bulk?)
+         (map #(entity-schema % sch services opts))
+         (apply merge {})))))
 
 (s/defschema EntityError
   "Error related to one entity of the bulk"
@@ -74,7 +82,10 @@
   (let [patchable-entities (into {}
                                  (filter #(:can-patch? (second %)))
                                  (get-entities services))]
-    (entities-bulk-schema patchable-entities :partial-schema services)))
+    (entities-bulk-schema patchable-entities
+                          :partial-schema
+                          services
+                          {:required-ids? true})))
 
 (s/defn BulkRefs :- (s/protocol s/Schema)
   [services :- GetEntitiesServices]
@@ -91,7 +102,13 @@
   [services :- GetEntitiesServices]
   (entities-bulk-schema (get-entities services) :new-schema services))
 
-(def BulkUpdate NewBulk)
+(s/defn BulkUpdate :- (s/protocol s/Schema)
+  "Returns NewBulk schema without disabled entities and required ids"
+  [services :- GetEntitiesServices]
+  (entities-bulk-schema (get-entities services)
+                        :new-schema
+                        services
+                        {:required-ids? true}))
 
 (def NewBulkDelete BulkRefs)
 

test/ctia/bulk/routes_test.clj

@@ -319,6 +319,7 @@
                             :headers {"Authorization" "45c1f5e3f05d0"})
              bulk-ids (:parsed-body response)
              show-props (get-http-show (app->HTTPShowServices app))]
+
          (is (= 201 (:status response)))
 
          (doseq [type (keys new-bulk)]
@@ -327,10 +328,10 @@
                     (count (get bulk-ids type))))))
          (testing "created events are generated"
            (let [expected-created-ids (mapcat val (dissoc bulk-ids :tempids))]
-               (check-events event-store
-                             ident
-                             expected-created-ids
-                             :record-created)))
+             (check-events event-store
+                           ident
+                           expected-created-ids
+                           :record-created)))
          (testing "GET /ctia/bulk"
            (let [{status :status
                   response :parsed-body}
@@ -387,6 +388,29 @@
                            (concat indicator-ids sighting-ids note-ids)
                            :record-updated)))
 
+         (testing "PUT /ctia/bulk without ids"
+           (let [update-fn (fn [record] (-> record
+                                            (assoc :source "updated")
+                                            (dissoc :id)))
+                 indicator-ids (:indicators bulk-ids)
+                 sighting-ids (:sightings bulk-ids)
+                 note-ids (:notes bulk-ids)
+                 update-bulk {:indicators (map (fn [_] (update-fn (mk-new-indicator 0))) indicator-ids)
+                              :sightings (map (fn [_] (update-fn (mk-new-sighting 0))) sighting-ids)
+                              :notes (map (fn [_] (update-fn (mk-new-note 0))) note-ids)}
+                 expected-update {:indicators {:updated (set indicator-ids)}
+                                  :sightings {:updated (set sighting-ids)}
+                                  :notes {:updated (set note-ids)}}
+                 {:keys [status parsed-body]} (PUT app
+                                                   bulk-url
+                                                   :form-params update-bulk
+                                                   :headers {"Authorization" "45c1f5e3f05d0"})]
+             (is (= 400 status))
+             (is (= parsed-body
+                    {:errors {:notes (repeat 7 {:id 'missing-required-key})
+                              :indicators (repeat 7 {:id 'missing-required-key})
+                              :sightings (repeat 7 {:id 'missing-required-key})}}))))
+
          (testing "PATCH /ctia/bulk"
            (let [incident-ids (:incidents bulk-ids)
                  sighting-ids (:sightings bulk-ids)
@@ -415,20 +439,29 @@
                             :headers {"Authorization" "45c1f5e3f05d0"})))
                  "only entities with patch-bulk? set to true can be submitted to PATCH bulk")))
 
+         (testing "PATCH /ctia/bulk without ids"
+           (let [patch-bulk {:incidents (repeat 5 (hash-map :source "patched"))}
+                 {:keys [status parsed-body]} (PATCH app
+                                                     bulk-url
+                                                     :form-params patch-bulk
+                                                     :headers {"Authorization" "45c1f5e3f05d0"})]
+             (is (= 400 status))
+             (is (= parsed-body {:errors {:incidents (repeat 5 {:id 'missing-required-key})}}))))
+
          (testing "DELETE /ctia/bulk"
            ;; edge cases are tested in bulk/core-test
            (let [delete-bulk-query (into {}
                                          (map (fn [[k ids]]
                                                 {k (take 2 ids)}))
                                          (dissoc bulk-ids :tempids))
                  expected-deleted (into {}
-                                       (map (fn [[k ids]]
-                                              {k {:deleted ids}}))
-                                       delete-bulk-query)
+                                        (map (fn [[k ids]]
+                                               {k {:deleted ids}}))
+                                        delete-bulk-query)
                  expected-not-found (into {}
-                                       (map (fn [[k ids]]
-                                              {k {:errors {:not-found ids}}}))
-                                       delete-bulk-query)
+                                          (map (fn [[k ids]]
+                                                 {k {:errors {:not-found ids}}}))
+                                          delete-bulk-query)
                  delete-bulk-url "ctia/bulk?wait_for=true"
                  {delete-status-1 :status delete-res-1 :parsed-body}
                  (DELETE app