Skip to content

Add SimpleHttpProvider for GraphClientFactory compatibility #38

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.

Sign up for GitHub

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

Jump to bottom
Merged
andrueastman merged 8 commits into from
Oct 3, 2019
Merged

Add SimpleHttpProvider for GraphClientFactory compatibility #38

andrueastman merged 8 commits into from
Oct 3, 2019

Conversation

@andrueastman
Copy link
Contributor

@andrueastman andrueastman commented Sep 25, 2019 •
edited
Loading

Changes proposed in this pull request

  • Adds a new IHttpProvider to enhance compatibility with GraphClientFactory by working towards adding support for the following scenario.
  HttpClient client = GraphClientFactory.Create(...);
  var graphServiceClient = new GraphServiceClient(client);

This means that one can easily use a custom httpClient with the request builders provided.

  • Modifies checks in BaseRequests.cs to not authenticate the requests when the Provider is
    either SimpleHttprovider or HttpProvider and when authprovider is present.
  • Adds tests

Other links

Andrew Omondi added 2 commits September 25, 2019 10:10
Add SimpleHttpProvider for GraphClientFactory compatibility
65b9a1a 
- Adds tests to validate similar behavior to HttpProvider
- Add modified checks to not authenticate SimpleHttprovider when authprovider is presents
- Fixed Ambiguous call in test.
Bumps up version number.
9803d50
@andrueastman andrueastman mentioned this pull request Sep 25, 2019
Merged
@andrueastman andrueastman self-assigned this Sep 25, 2019
@MIchaelMainer MIchaelMainer added this to the 1.18.0 milestone Sep 25, 2019
MIchaelMainer
MIchaelMainer requested changes Sep 25, 2019
View reviewed changes
Copy link
Collaborator

@MIchaelMainer MIchaelMainer left a comment

Choose a reason for hiding this comment

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

This PR is dependent on #37

@andrueastman
Copy link
Contributor Author

andrueastman commented Sep 26, 2019 •
edited
Loading

We will pull this in once #37 is completed and I have pulled in any necessary error/diagnostic changes

@andrueastman
Copy link
Contributor Author

andrueastman commented Oct 1, 2019

New support for error/diagnostic info has now been incorporated. This PR should now be ready to be pulled in.

@MIchaelMainer @darrelmiller @irvinesunday

@MIchaelMainer
Copy link
Collaborator

MIchaelMainer commented Oct 1, 2019

It looks fine - just set the version back to 1.18.0 and then we should be set. Part of me wants to turn some of those thrown ServiceException into ClientException as they aren't really ServiceException. Perhaps we do that wholesale at a later break.

@andrueastman
Copy link
Contributor Author

andrueastman commented Oct 1, 2019

Version number has now been corrected to 1.18.0

@darrelmiller
Copy link
Contributor

darrelmiller commented Oct 1, 2019

Looks like merging the two branches broke some tests. Seems related to ThrowSite. /cc @andrueastman @MIchaelMainer

@andrueastman
Copy link
Contributor Author

andrueastman commented Oct 2, 2019

Thanks pointing this out. This has been resolved.

To prevent this from occurring again, I think we might need to look into the pr validation checks as it looks to me like the tests are not really run.

@andrueastman andrueastman merged commit b433dd7 into dev Oct 3, 2019
@andrueastman andrueastman deleted the andrueastman/simplehttpprovider branch October 3, 2019 17:53
@fowl2 fowl2 mentioned this pull request Dec 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MIchaelMainer MIchaelMainer MIchaelMainer approved these changes

@irvinesunday irvinesunday Awaiting requested review from irvinesunday

@darrelmiller darrelmiller Awaiting requested review from darrelmiller

Assignees

@andrueastman andrueastman

Labels

None yet

Projects

None yet

Milestone

1.18.0

Development

Successfully merging this pull request may close these issues.

4 participants

@andrueastman @MIchaelMainer @darrelmiller