Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMR-9848: When submitting multiple associations, if one fails CMR returns a 400 but still makes the other associations #2174

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
200a41e
CMR-9429: Validate ProcessingLevel Id
Jun 5, 2024
3a044b4
CMR-9429: Revert the commit into master by accident
Jun 5, 2024
8ad1e4d
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jun 10, 2024
bf1b84c
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jun 11, 2024
5190c5a
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jun 19, 2024
eae148e
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jun 21, 2024
3e669b0
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jul 17, 2024
8ee5022
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jul 22, 2024
0c88ff3
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jul 25, 2024
9fe05ac
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Jul 31, 2024
2dba0d6
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Aug 8, 2024
393cdcd
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Aug 12, 2024
d274599
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Aug 29, 2024
af1dc64
Merge branch 'master' of https://github.com/nasa/Common-Metadata-Repo…
Sep 13, 2024
64ee76f
CMR-9848 Return 400 failure status only when all the associations/dis…
Sep 16, 2024
61eb516
CMR-9848: Fixed some tests
Sep 16, 2024
f46c405
CMR-9848:
Sep 18, 2024
cfffaa6
CMR-9848:
Sep 18, 2024
55ba3e1
CMR-9848: Modified the api doc
Sep 18, 2024
5a4f989
CMR-9848: Modified API doc
Sep 18, 2024
a4dab32
CMR-9848: Removed print.
Sep 18, 2024
3c38e61
CMR-9848: Fixed api doc.
Sep 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions search-app/docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3857,7 +3857,7 @@ curl -XDELETE -i -H "Content-Type: application/json" -H "Authorization: Bearer X
{"concept_id": "C1200000006-PROV1"},
{"concept_id": "C1200000007-PROV1"}]'

HTTP/1.1 400 Bad Request
HTTP/1.1 400 Bad Request
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
HTTP/1.1 400 Bad Request
HTTP/1.1 200 Bad Request```
Isn't this example also `200` since the `C1200000005-PROV1` in the example would have succeeded?

Content-Type: application/json;charset=ISO-8859-1
Content-Length: 168

Expand Down Expand Up @@ -3890,7 +3890,7 @@ Content-Length: 168
]
```

On occasions when tag dissociation cannot be processed at all due to invalid input, tag dissociation request will return a failure status code with the appropriate error message.
On occasions when tag dissociation cannot be processed at all due to invalid input, tag dissociation request will return status code 200 with appropriate error messages for each failed dissociation. If all the dissociations in the request fail, a 400 failure status code will be returned.

#### <a name="dissociating-collections-with-a-tag-by-query"></a> Dissociating a Tag from Collections by query

Expand Down Expand Up @@ -4653,7 +4653,7 @@ curl -XPOST -i -H "Content-Type: application/json" -H "Authorization: Bearer XXX
'[{"concept_id": "C1200000005-PROV1", "data": {"order_option": "OO1200445588-PROV1"}},
{"concept_id": "C1200000006-PROV1"}]'

HTTP/1.1 400 BAD REQUEST
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Content-Length: 168

Expand All @@ -4678,20 +4678,21 @@ Content-Length: 168
]
```

On occasions when service association cannot be processed at all due to invalid input, service association request will return a failure status code with the appropriate error message.
On occasions when service association cannot be processed at all due to invalid input, service association request will return status code 200 with appropriate error messages for each failed association. If all the associations in the request fail, a 400 failure status code will be returned.

#### <a name="service-dissociation"></a> Service Dissociation

A service identified by its concept id can be dissociated from collections through a list of collection concept revisions similar to service association requests.
Service dissociation requires that user has update permission on INGEST_MANAGEMENT_ACL for either the collection's provider, or the service's provider.

Service dissociation
```
curl -XDELETE -i -H "Content-Type: application/json" -H "Authorization: Bearer XXXXX" %CMR-ENDPOINT%/services/S1200000008-PROV1/associations -d \
'[{"concept_id": "C1200000005-PROV1"},
{"concept_id": "C1200000006-PROV1"},
{"concept_id": "C1200000007-PROV1"}]'

HTTP/1.1 400 BAD REQUEST
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Content-Length: 168

Expand Down Expand Up @@ -4732,7 +4733,7 @@ Content-Length: 168
]
```

On occasions when service dissociation cannot be processed at all due to invalid input, service dissociation request will return a failure status code with the appropriate error message.
On occasions when service dissociation cannot be processed at all due to invalid input, service dissociation request will return status code 200 with appropriate error messages for each failed dissociation. If all the dissociations in the request fail, a 400 failure status code will be returned.

### <a name="tool"></a> Tool

Expand Down Expand Up @@ -5030,7 +5031,7 @@ curl -XPOST -i -H "Content-Type: application/json" -H "Authorization: Bearer XXX
'[{"concept_id": "C1200000005-PROV1"},
{"concept_id": "C1200000006-PROV1"}]'

HTTP/1.1 400 BAD REQUEST
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Content-Length: 168

Expand Down Expand Up @@ -5064,7 +5065,7 @@ Content-Length: 168
]
```

On occasions when tool association cannot be processed at all due to invalid input, tool association request will return a failure status code with the appropriate error message.
On occasions when tool association cannot be processed at all due to invalid input, tool association request will return status code 200 with appropriate error messages for each failed association. If all the associations in the request fail, a 400 failure status code will be returned.

#### <a name="tool-dissociation"></a> Tool Dissociation

Expand All @@ -5077,7 +5078,7 @@ curl -XDELETE -i -H "Content-Type: application/json" -H "Authorization: Bearer X
{"concept_id": "C1200000006-PROV1"},
{"concept_id": "C1200000007-PROV1"}]'

HTTP/1.1 400 BAD REQUEST
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Content-Length: 168

Expand Down Expand Up @@ -5118,7 +5119,7 @@ Content-Length: 168
]
```

On occasions when tool dissociation cannot be processed at all due to invalid input, tool dissociation request will return a failure status code with the appropriate error message.
On occasions when tool dissociation cannot be processed at all due to invalid input, tool dissociation request will return status code 200 with appropriate error messages for each failed dissociation. If all the associations in the request fail, a 400 failure status code will be returned.

### <a name="subscription"></a> Subscription

Expand Down
8 changes: 4 additions & 4 deletions search-app/src/cmr/search/api/association.clj
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,18 @@
:body (json/generate-string (util/snake-case-data data))
:headers {"Content-Type" mt/json}}))

(defn- results-contain-errors?
"Returns true if the results contain :errors"
(defn- all-results-contain-errors?
"Returns true if the all results contain :errors"
[results]
(seq (filter #(some? (:errors %)) results)))
(not (some #(nil? (:errors %)) results)))

(defn association-results->status-code
"Check for concept-types requiring error status to be returned. This is currently :service and :variable
If the concept-type is error-sensitive the function will check for any errors in the results, and will return 400 if
any are errors are present. Otherwise it will return 200"
[concept-type results]
(if (some #{concept-type} '(:variable :service :tool))
(if (results-contain-errors? results)
(if (all-results-contain-errors? results)
400
200)
200))
Expand Down
8 changes: 4 additions & 4 deletions search-app/src/cmr/search/api/generic_association.clj
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@
:body (json/generate-string (util/snake-case-data data))
:headers {"Content-Type" mt/json}}))

(defn- results-contain-errors?
"Returns true if the results contain :errors"
(defn- all-results-contain-errors?
"Returns true if every single result contains :errors"
[results]
(seq (filter #(some? (:errors %)) results)))
(not (some #(nil? (:errors %)) results)))

(defn- results->status-code
"Return status code depending on if results contains error."
[results]
(if (results-contain-errors? results)
(if (all-results-contain-errors? results)
400
200))

Expand Down
5 changes: 5 additions & 0 deletions system-int-test/src/cmr/system_int_test/utils/tool_util.clj
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,11 @@
(is (= (set (map :concept-id expected-colls))
(set colls)))))

(defn assert-tool-dissociation-response-ok?
"Assert the tool association response when status code is 200 is correct."
[coll-tool-associations response]
(assert-tool-association-response-ok? coll-tool-associations response false))

(defn assert-tool-dissociation-bad-request
"Assert the tool association response when status code is 400 is correct."
[coll-tool-associations response]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@
:revision-id coll2-revision-id}])
tla2-concept-id (some #(when (:tool-association %) (get-in % [:tool-association :concept-id]))
(:body assoc-response2))]
(is (= {:status 400, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id tla1-concept-id, :revision-id 1}, :associated-item {:concept-id coll2-concept-id, :revision-id coll2-revision-id}}]}
(is (= {:status 200, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id tla1-concept-id, :revision-id 1}, :associated-item {:concept-id coll2-concept-id, :revision-id coll2-revision-id}}]}
assoc-response1))

(is (= {:status 400, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id tla2-concept-id, :revision-id 1}, :associated-item {:concept-id coll2-concept-id, :revision-id coll2-revision-id}}]}
(is (= {:status 200, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id tla2-concept-id, :revision-id 1}, :associated-item {:concept-id coll2-concept-id, :revision-id coll2-revision-id}}]}
assoc-response2))))

(testing "dissociation-permission-test"
Expand Down Expand Up @@ -118,7 +118,7 @@
[{:concept-id coll1-concept-id}
{:concept-id coll2-concept-id
:revision-id coll2-revision-id}])]
(is (= {:status 400, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] or provider of service/tool to delete the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id (last tla1-concept-ids), :revision-id 3}, :associated-item {:concept-id coll2-concept-id, :revision-id 1}}]}
(is (= {:status 200, :body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] or provider of service/tool to delete the association.")], :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id (last tla1-concept-ids), :revision-id 3}, :associated-item {:concept-id coll2-concept-id, :revision-id 1}}]}
dissoc-response1))

(is (= {:status 200, :body [{:tool-association {:concept-id (first tla2-concept-ids), :revision-id 2}, :associated-item {:concept-id coll1-concept-id}} {:tool-association {:concept-id (last tla2-concept-ids), :revision-id 3}, :associated-item {:concept-id coll2-concept-id, :revision-id 1}}]}
Expand All @@ -143,14 +143,14 @@
:revision-id coll2-revision-id}])
sa2-concept-id (some #(when (:service-association %) (get-in % [:service-association :concept-id]))
(:body assoc-response2))]
(is (= {:status 400
(is (= {:status 200
:body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")]
:associated-item {:concept-id coll1-concept-id}}
{:service-association {:concept-id sa1-concept-id, :revision-id 1}
:associated-item {:concept-id coll2-concept-id, :revision-id coll2-revision-id}}]}
assoc-response1))

(is (= {:status 400
(is (= {:status 200
:body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] to make the association.")]
:associated-item {:concept-id coll1-concept-id}}
{:service-association {:concept-id sa2-concept-id, :revision-id 1}
Expand Down Expand Up @@ -191,7 +191,7 @@
[{:concept-id coll1-concept-id}
{:concept-id coll2-concept-id
:revision-id coll2-revision-id}])]
(is (= {:status 400
(is (= {:status 200
:body [{:errors [(str "User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [" coll1-concept-id "] or provider of service/tool to delete the association.")]
:associated-item {:concept-id coll1-concept-id}}
{:service-association {:concept-id (last sa1-concept-ids), :revision-id 3}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
(let [response (association-util/associate-by-concept-ids
token concept-id [{:concept-id c2-p1}
{:concept-id "C100-P5"}])]
(service-util/assert-service-association-bad-request
(service-util/assert-service-association-response-ok?
{[c2-p1] {:concept-id "SA1200000028-CMR"
:revision-id 1}
["C100-P5"] {:errors ["User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [C100-P5] to make the association."]}}
Expand Down Expand Up @@ -302,7 +302,7 @@
{:concept-id (:concept-id coll2) :revision-id 1} ;; success
{:concept-id (:concept-id coll3)}])] ;; no service association

(service-util/assert-service-dissociation-bad-request
(service-util/assert-service-dissociation-response-ok?
{["C100-P5"] {:errors ["Collection [C100-P5] does not exist or is not visible."]}
["C1200000012-PROV1"] {:concept-id "SA1200000016-CMR" :revision-id 2}
["C1200000013-PROV1" 1] {:concept-id "SA1200000017-CMR" :revision-id 2}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
(let [response (association-util/associate-by-concept-ids
token concept-id [{:concept-id c2-p1}
{:concept-id "C100-P5"}])]
(tool-util/assert-tool-association-bad-request
(tool-util/assert-tool-association-response-ok?
{[c2-p1] {:concept-id "TLA1200000028-CMR"
:revision-id 1}
["C100-P5"] {:errors ["User doesn't have update permission on INGEST_MANAGEMENT_ACL for provider of collection [C100-P5] to make the association."]}}
Expand Down Expand Up @@ -301,7 +301,7 @@
{:concept-id (:concept-id coll2) :revision-id 1} ;; success
{:concept-id (:concept-id coll3)}])] ;; no tool association

(tool-util/assert-tool-dissociation-bad-request
(tool-util/assert-tool-dissociation-response-ok?
{["C100-P5"] {:errors ["Collection [C100-P5] does not exist or is not visible."]}
["C1200000012-PROV1"] {:concept-id "TLA1200000016-CMR" :revision-id 2}
["C1200000013-PROV1" 1] {:concept-id "TLA1200000017-CMR" :revision-id 2}
Expand Down