-
Notifications
You must be signed in to change notification settings - Fork 735
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
Expose Headers #339
Expose Headers #339
Conversation
Signed-off-by: Kanstantsin Shautsou <[email protected]>
|
||
/** | ||
* Most (all?) domain objects in GitHub seems to have these 4 properties. | ||
*/ | ||
@SuppressFBWarnings(value = {"UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD", "UWF_UNWRITTEN_FIELD", | ||
"NP_UNWRITTEN_FIELD"}, justification = "JSON API") | ||
public abstract class GHObject { | ||
// not data but information related to data from responce |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe even GHObject
should extend GHResponse
GH has specific to GET/POST headers required for analysing in case of error. Signed-off-by: Kanstantsin Shautsou <[email protected]>
true | ||
); | ||
} catch (IOException ex) { | ||
assertThat(ex, instanceOf(GHFileNotFoundException.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is ExpectedExceptionRule for that purposes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that requires it to be rethrown.
And the next few assertions build upon it being an instance of that class, so otherwise it's a CCE.
So having a clear error message isn't that bad IMO.
gitHub.getMyself(); | ||
|
||
// we request read | ||
final List<String> scopes = Arrays.asList("repo", "read:org", "user:email", "read:repo_hook"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final
in tests looks like rubbish and doesn't add anything
return responceHeaderFields; | ||
} | ||
|
||
public GHIOException withResponceHeaderFields(HttpURLConnection urlConnection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KostyaSha this typo was not addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it before the merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it before the merge
@kohsuke ? |
Seems like the wrong approach adding the headers as fields to the objects. A better approach might be to set a thread-local in the 'GitHub' that holds the header values from the last api call. Otherwise the "live Object view" nature of this api would be polluted. One can argue whether the "live object view" that hides the nature of the api calls with a layer of abstraction is a good fit for accessing GitHub, but it is the philosophy of this library, so absence a counter argument showing why the headers can remain stale, I am inclined to recommend against this change (but by all means present your counter-argument if you believe it valid, I am not closed to changing my mind) |
Oauth header info is part of response data for real object and exception, not Thread Local data. If i right understand threadLocal data will be lost in runnables/queues and last value may be/will be overwritten from parallel GH client calls while returned object will have correct value. I accept that i exposed too much headers and it acceptable to expose only useful headers to end user like X-oauth scope requested/required. |
ping |
? |
Fixes #303
Bit shocked with design, this info is needed for normal and failed requests, so added GHObject field injector and wrapped exceptions. Not sure only whether it possible to change throwing exception types without breaking compatibility.
cc @stephenc your github-branch-org-source should be also affected if it close to non single admin token use case.
This change is