Skip to content

Commit 621a7a6

Browse files
timrogersArin Ghazarian
timrogers
and
Arin Ghazarian
authored
Consistently convert GraphQL errors to OctoshiftCliExceptions with helpful error messages across GithubApi (#868)
In `GithubApi`, we have the helpful `EnsureSuccessGraphQLResponse` helper which checks the GraphQL response for errors, and if an error is found, it raises a `OctoshiftCliException`. Currently, this helper is not used consistently across GraphQL API calls, which means that we have some cases where the GraphQL API returns an error and the CLI crashes with an unhelpful `System.InvalidOperationException: Cannot access child value on Newtonsoft.Json.Linq.JValue.`. To find out what actually went wrong, the user has to either: * (a) use the `--verbose` option or * (b) manually check the verbose log file This PR creates a new `PostGraphQLAsync` method on `GitHubClient` which includes the error handling logic from `EnsureSuccessGraphQLResponse`. We then use the `PostGraphQLAsync` method instead of `PostAsync` for GraphQL requests, which means that we get this consistent error handling for free. I do not apply this to `ReclaimMannequin` (which has its own error handling) and `GetMannequins` (which has some complex pagination logic which I didn't want to mess with right now). Fixes #867. <!-- For the checkboxes below you must check each one to indicate that you either did the relevant task, or considered it and decided there was nothing that needed doing --> - [x] Did you write/update appropriate tests - [x] Release notes updated (if appropriate) - [x] Appropriate logging output - [x] Issue linked - [ ] Docs updated (or issue created) - [ ] New package licenses are added to `ThirdPartyNotices.txt` (if applicable) <!-- For docs we should review the docs at: https://docs.github.com/en/early-access/github/migrating-with-github-enterprise-importer and the README.md in this repo If a doc update is required based on the changes in this PR, it is sufficient to create an issue and link to it here. The doc update can be made later/separately. The process to update the docs can be found here: https://github.com/github/docs-early-access#opening-prs The markdown files are here: https://github.com/github/docs-early-access/tree/main/content/github/migrating-with-github-enterprise-importer --> --------- Co-authored-by: Arin Ghazarian <aringhazarian@github.com>
1 parent ae773bb commit 621a7a6

File tree

7 files changed

+280
-280
lines changed

7 files changed

+280
-280
lines changed

‎RELEASENOTES.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
- Adds retry logic during GHES archive generation in cases of transient failure
22
- Added log output linking to migration log URL after migration completes
33
- Add support for specifying `--archive-download-host` with `gh bbs2gh migrate-repo` and `gh bbs2gh generate-script`, rather than taking the host from the `--bbs-server-url`
4+
- Improve handling of GraphQL errors, throwing an exception with the specific error message returned by the API

‎src/Octoshift/GithubApi.cs‎

Lines changed: 14 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -167,10 +167,7 @@ public virtual async Task<string> GetOrganizationId(string org)
167167

168168
var response = await _retryPolicy.Retry(async () =>
169169
{
170-
var httpResponse = await _client.PostAsync(url, payload);
171-
var data = JObject.Parse(httpResponse);
172-
173-
EnsureSuccessGraphQLResponse(data);
170+
var data = await _client.PostGraphQLAsync(url, payload);
174171

175172
return (string)data["data"]["organization"]["id"];
176173
});
@@ -192,10 +189,7 @@ public virtual async Task<string> GetEnterpriseId(string enterpriseName)
192189

193190
var response = await _retryPolicy.Retry(async () =>
194191
{
195-
var httpResponse = await _client.PostAsync(url, payload);
196-
var data = JObject.Parse(httpResponse);
197-
198-
EnsureSuccessGraphQLResponse(data);
192+
var data = await _client.PostGraphQLAsync(url, payload);
199193

200194
return (string)data["data"]["enterprise"]["id"];
201195
});
@@ -227,8 +221,7 @@ public virtual async Task<string> CreateAdoMigrationSource(string orgId, string
227221
operationName = "createMigrationSource"
228222
};
229223

230-
var response = await _client.PostAsync(url, payload);
231-
var data = JObject.Parse(response);
224+
var data = await _client.PostGraphQLAsync(url, payload);
232225

233226
return (string)data["data"]["createMigrationSource"]["migrationSource"]["id"];
234227
}
@@ -253,8 +246,7 @@ public virtual async Task<string> CreateBbsMigrationSource(string orgId)
253246
operationName = "createMigrationSource"
254247
};
255248

256-
var response = await _client.PostAsync(url, payload);
257-
var data = JObject.Parse(response);
249+
var data = await _client.PostGraphQLAsync(url, payload);
258250

259251
return (string)data["data"]["createMigrationSource"]["migrationSource"]["id"];
260252
}
@@ -279,8 +271,7 @@ public virtual async Task<string> CreateGhecMigrationSource(string orgId)
279271
operationName = "createMigrationSource"
280272
};
281273

282-
var response = await _client.PostAsync(url, payload);
283-
var data = JObject.Parse(response);
274+
var data = await _client.PostGraphQLAsync(url, payload);
284275

285276
return (string)data["data"]["createMigrationSource"]["migrationSource"]["id"];
286277
}
@@ -351,10 +342,7 @@ mutation startRepositoryMigration(
351342
operationName = "startRepositoryMigration"
352343
};
353344

354-
var response = await _client.PostAsync(url, payload);
355-
var data = JObject.Parse(response);
356-
357-
EnsureSuccessGraphQLResponse(data);
345+
var data = await _client.PostGraphQLAsync(url, payload);
358346

359347
return (string)data["data"]["startRepositoryMigration"]["repositoryMigration"]["id"];
360348
}
@@ -395,10 +383,7 @@ mutation startOrganizationMigration (
395383
operationName = "startOrganizationMigration"
396384
};
397385

398-
var response = await _client.PostAsync(url, payload);
399-
var data = JObject.Parse(response);
400-
401-
EnsureSuccessGraphQLResponse(data);
386+
var data = await _client.PostGraphQLAsync(url, payload);
402387

403388
return (string)data["data"]["startOrganizationMigration"]["orgMigration"]["id"];
404389
}
@@ -414,10 +399,7 @@ mutation startOrganizationMigration (
414399

415400
var response = await _retryPolicy.Retry(async () =>
416401
{
417-
var httpResponse = await _client.PostAsync(url, payload);
418-
var data = JObject.Parse(httpResponse);
419-
420-
EnsureSuccessGraphQLResponse(data);
402+
var data = await _client.PostGraphQLAsync(url, payload);
421403

422404
return (
423405
State: (string)data["data"]["node"]["state"],
@@ -458,10 +440,7 @@ public virtual async Task<string> StartBbsMigration(string migrationSourceId, st
458440

459441
var response = await _retryPolicy.Retry(async () =>
460442
{
461-
var httpResponse = await _client.PostAsync(url, payload);
462-
var data = JObject.Parse(httpResponse);
463-
464-
EnsureSuccessGraphQLResponse(data);
443+
var data = await _client.PostGraphQLAsync(url, payload);
465444

466445
return (
467446
State: (string)data["data"]["node"]["state"],
@@ -494,10 +473,7 @@ public virtual async Task<string> GetMigrationLogUrl(string org, string repo)
494473

495474
var response = await _retryPolicy.Retry(async () =>
496475
{
497-
var httpResponse = await _client.PostAsync(url, payload);
498-
var data = JObject.Parse(httpResponse);
499-
500-
EnsureSuccessGraphQLResponse(data);
476+
var data = await _client.PostGraphQLAsync(url, payload);
501477

502478
var nodes = (JArray)data["data"]["organization"]["repositoryMigrations"]["nodes"];
503479

@@ -555,8 +531,7 @@ public virtual async Task<bool> GrantMigratorRole(string org, string actor, stri
555531

556532
try
557533
{
558-
var response = await _client.PostAsync(url, payload);
559-
var data = JObject.Parse(response);
534+
var data = await _client.PostGraphQLAsync(url, payload);
560535

561536
return (bool)data["data"]["grantMigratorRole"]["success"];
562537
}
@@ -582,8 +557,7 @@ public virtual async Task<bool> RevokeMigratorRole(string org, string actor, str
582557

583558
try
584559
{
585-
var response = await _client.PostAsync(url, payload);
586-
var data = JObject.Parse(response);
560+
var data = await _client.PostGraphQLAsync(url, payload);
587561

588562
return (bool)data["data"]["revokeMigratorRole"]["success"];
589563
}
@@ -697,10 +671,9 @@ public virtual async Task<string> GetUserId(string login)
697671
};
698672

699673
// TODO: Add retry logic here, but need to inspect the actual error message and differentiate between transient failure vs user doesn't exist (only retry on failure)
700-
var response = await _client.PostAsync(url, payload);
701-
var data = JObject.Parse(response);
674+
var data = await _client.PostGraphQLAsync(url, payload);
702675

703-
return data["data"]["user"].Any() ? (string)data["data"]["user"]["id"] : null;
676+
return (string)data["data"]["user"]["id"];
704677
}
705678

706679
public virtual async Task<MannequinReclaimResult> ReclaimMannequin(string orgId, string mannequinId, string targetUserId)
@@ -828,16 +801,6 @@ private static Mannequin BuildMannequin(JToken mannequin)
828801
};
829802
}
830803

831-
private void EnsureSuccessGraphQLResponse(JObject response)
832-
{
833-
if (response.TryGetValue("errors", out var jErrors) && jErrors is JArray { Count: > 0 } errors)
834-
{
835-
var error = (JObject)errors[0];
836-
var errorMessage = error.TryGetValue("message", out var jMessage) ? (string)jMessage : null;
837-
throw new OctoshiftCliException($"{errorMessage ?? "UNKNOWN"}");
838-
}
839-
}
840-
841804
private static GithubSecretScanningAlert BuildSecretScanningAlert(JToken secretAlert) =>
842805
new()
843806
{

‎src/Octoshift/GithubClient.cs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,18 @@ public virtual async IAsyncEnumerable<JToken> GetAllAsync(string url, Dictionary
7373
public virtual async Task<string> PostAsync(string url, object body, Dictionary<string, string> customHeaders = null) =>
7474
(await SendAsync(HttpMethod.Post, url, body, customHeaders: customHeaders)).Content;
7575

76+
public virtual async Task<JToken> PostGraphQLAsync(
77+
string url,
78+
object body,
79+
Dictionary<string, string> customHeaders = null)
80+
{
81+
var response = await PostAsync(url, body, customHeaders);
82+
var data = JObject.Parse(response);
83+
EnsureSuccessGraphQLResponse(data);
84+
85+
return data;
86+
}
87+
7688
public virtual async IAsyncEnumerable<JToken> PostGraphQLWithPaginationAsync(
7789
string url,
7890
object body,
@@ -237,5 +249,15 @@ private void SetRetryDelay(IEnumerable<KeyValuePair<string, IEnumerable<string>>
237249

238250
_retryDelay = (int)(resetUnixSeconds - currentUnixSeconds);
239251
}
252+
253+
private void EnsureSuccessGraphQLResponse(JObject response)
254+
{
255+
if (response.TryGetValue("errors", out var jErrors) && jErrors is JArray { Count: > 0 } errors)
256+
{
257+
var error = (JObject)errors[0];
258+
var errorMessage = error.TryGetValue("message", out var jMessage) ? (string)jMessage : null;
259+
throw new OctoshiftCliException($"{errorMessage ?? "UNKNOWN"}");
260+
}
261+
}
240262
}
241263
}

‎src/Octoshift/ReclaimService.cs‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,10 +99,6 @@ public virtual async Task ReclaimMannequin(string mannequinUser, string mannequi
9999
}
100100

101101
var targetUserId = await _githubApi.GetUserId(targetUser);
102-
if (targetUserId == null)
103-
{
104-
throw new OctoshiftCliException($"Target user {targetUser} not found.");
105-
}
106102

107103
var success = true;
108104

0 commit comments

Comments
 (0)