Gitlab has a weird way of returning error messages from the API.
There are multiple kinds of objects with different keys depending
on what error happened.

Previously we were only looking for the "message" key in the returned
error message object. Now we look for multiple kinds of keys, extract
them all and choose the most sensible one.

This should cover many more cases when Gitlab returns errors.

I have also added a few tests that ensure this behaviour.

Somewhat related to b729ac0800b8b0b1ec61b9ac3cc37e78f320c14d.

Signed-off-by: Nico Sonack <nsonack@herrhotzenplotz.de>
---
 Makefile.am                                  |  2 ++
 include/gcli/gitlab/api.h                    | 10 ++++++
 src/gitlab/api.c                             | 20 ++++++++++-
 templates/gitlab/api.t                       |  8 ++++-
 tests/gitlab-parse-tests.c                   | 35 ++++++++++++++++++++
 tests/samples/gitlab_error_unauthorised.json |  1 +
 tests/samples/gitlab_token_expired.json      |  1 +
 7 files changed, 75 insertions(+), 2 deletions(-)
 create mode 100644 tests/samples/gitlab_error_unauthorised.json
 create mode 100644 tests/samples/gitlab_token_expired.json

diff --git a/Makefile.am b/Makefile.am
index 2c09679..d23b9ad 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -342,6 +342,8 @@ EXTRA_DIST += tests/samples/github_simple_comment.json	\
 	tests/samples/gitlab_simple_release.json	\
 	tests/samples/gitlab_simple_repo.json		\
 	tests/samples/gitlab_simple_snippet.json	\
+	tests/samples/gitlab_token_expired.json         \
+	tests/samples/gitlab_error_unauthorised.json    \
 	tests/samples/github_simple_check.json		\
 	tests/samples/gitea_simple_notification.json    \
 	tests/samples/bugzilla_attachments.json         \
diff --git a/include/gcli/gitlab/api.h b/include/gcli/gitlab/api.h
index be6cfde..39b4b9f 100644
--- a/include/gcli/gitlab/api.h
+++ b/include/gcli/gitlab/api.h
@@ -36,6 +36,16 @@
 
 #include <gcli/curl.h>
 
+/* Gitlab returns error messages in many strange ways. Since the
+ * keys are different we try to produce a good error message by looking
+ * for all possible keys and then returning a sensible value. This struct
+ * contains a collection of found messages. */
+struct gitlab_error_data {
+	char *message;
+	char *error_description;
+	char *error;
+};
+
 char const *gitlab_api_error_string(struct gcli_ctx *ctx, struct gcli_fetch_buffer *buf);
 int gitlab_user_id(struct gcli_ctx *ctx, char const *user_name);
 
diff --git a/src/gitlab/api.c b/src/gitlab/api.c
index ea87a74..285f7a1 100644
--- a/src/gitlab/api.c
+++ b/src/gitlab/api.c
@@ -40,12 +40,30 @@ gitlab_api_error_string(struct gcli_ctx *ctx, struct gcli_fetch_buffer *const bu
 	char *msg = NULL;
 	int rc;
 	struct json_stream stream = {0};
+	struct gitlab_error_data error_data = {0};
 
 	json_open_buffer(&stream, buf->data, buf->length);
-	rc = parse_gitlab_get_error(ctx, &stream, &msg);
+	rc = parse_gitlab_get_error(ctx, &stream, &error_data);
 	json_close(&stream);
 
+	/* Extract a most useful error message */
+	if (error_data.error_description) {
+		msg = strdup(error_data.error_description);
+	} else if (error_data.message) {
+		msg = strdup(error_data.message);
+	} else if (error_data.error) {
+		msg = strdup(error_data.error);
+	} else {
+		msg = NULL;
+	}
+
+	free(error_data.error_description);
+	free(error_data.message);
+	free(error_data.error);
+
 	if (rc < 0 || msg == NULL) {
+		free(msg);
+
 		if (sn_verbose()) {
 			return sn_asprintf("Could not parse Gitlab error response. "
 			                   "The response was:\n\n%.*s\n",
diff --git a/templates/gitlab/api.t b/templates/gitlab/api.t
index 605b612..14e4385 100644
--- a/templates/gitlab/api.t
+++ b/templates/gitlab/api.t
@@ -1 +1,7 @@
-parser gitlab_get_error is object of char* select "message" as string;
+include "gcli/gitlab/api.h";
+
+parser gitlab_get_error is
+object of struct gitlab_error_data with
+	("error_description" => error_description as string,
+	 "error" => error as string,
+	 "message" => message as string);
diff --git a/tests/gitlab-parse-tests.c b/tests/gitlab-parse-tests.c
index a815348..cc77b39 100644
--- a/tests/gitlab-parse-tests.c
+++ b/tests/gitlab-parse-tests.c
@@ -12,6 +12,7 @@
 #include <string.h>
 
 #include <gcli/ctx.h>
+#include <gcli/gitlab/api.h>
 
 #include "gcli_tests.h"
 
@@ -310,6 +311,38 @@ ATF_TC_BODY(gitlab_simple_snippet, tc)
 	gcli_destroy(&ctx);
 }
 
+ATF_TC_WITHOUT_HEAD(gitlab_error_token_expired);
+ATF_TC_BODY(gitlab_error_token_expired, tc)
+{
+	struct gcli_ctx *ctx = test_context();
+	struct gcli_fetch_buffer buffer = {0};
+	char const *errmsg = NULL;
+
+	buffer.length = sn_read_file(TESTSRCDIR"/samples/gitlab_token_expired.json",
+	                             &buffer.data);
+
+	ATF_REQUIRE(buffer.length >= 0);
+	errmsg = gitlab_api_error_string(ctx, &buffer);
+
+	ATF_CHECK_STREQ(errmsg, "Token has expired.");
+}
+
+ATF_TC_WITHOUT_HEAD(gitlab_error_unauthorised);
+ATF_TC_BODY(gitlab_error_unauthorised, tc)
+{
+	struct gcli_ctx *ctx = test_context();
+	struct gcli_fetch_buffer buffer = {0};
+	char const *errmsg = NULL;
+
+	buffer.length = sn_read_file(TESTSRCDIR"/samples/gitlab_error_unauthorised.json",
+	                             &buffer.data);
+
+	ATF_REQUIRE(buffer.length >= 0);
+	errmsg = gitlab_api_error_string(ctx, &buffer);
+
+	ATF_CHECK_STREQ(errmsg, "401 Unauthorized");
+}
+
 ATF_TP_ADD_TCS(tp)
 {
 	ATF_TP_ADD_TC(tp, gitlab_simple_fork);
@@ -321,6 +354,8 @@ ATF_TP_ADD_TCS(tp)
 	ATF_TP_ADD_TC(tp, gitlab_simple_release);
 	ATF_TP_ADD_TC(tp, gitlab_simple_repo);
 	ATF_TP_ADD_TC(tp, gitlab_simple_snippet);
+	ATF_TP_ADD_TC(tp, gitlab_error_token_expired);
+	ATF_TP_ADD_TC(tp, gitlab_error_unauthorised);
 
 	return atf_no_error();
 }
diff --git a/tests/samples/gitlab_error_unauthorised.json b/tests/samples/gitlab_error_unauthorised.json
new file mode 100644
index 0000000..e4d8208
--- /dev/null
+++ b/tests/samples/gitlab_error_unauthorised.json
@@ -0,0 +1 @@
+{ "message": "401 Unauthorized" }
diff --git a/tests/samples/gitlab_token_expired.json b/tests/samples/gitlab_token_expired.json
new file mode 100644
index 0000000..65bc049
--- /dev/null
+++ b/tests/samples/gitlab_token_expired.json
@@ -0,0 +1 @@
+{ "error": "token_expired", "error_description": "Token has expired." }
-- 
2.44.0

Nico Sonack, May 17, 2024 at 15:08:
> Somewhat related to b729ac0800b8b0b1ec61b9ac3cc37e78f320c14d.
When referencing other commits, we can ask git to provide some more information
which adds context and is useful for commit message--or indeed just mentioning a
commit in general.

  $ git log -1 --format=reference b729ac0800b8b0b1ec61b9ac3cc37e78f320c14d
  b729ac0 (Prevent segmentation fault in 404 cases on Gitlab, 2023-12-23)

I have applied the patch, and pushed to all remotes. Thanks!

- g