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.

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

#2294 Make Http.Client projects usable by traditional netfx projects #2867

Merged
merged 6 commits into from
Mar 3, 2020
Merged

#2294 Make Http.Client projects usable by traditional netfx projects #2867

merged 6 commits into from
Mar 3, 2020

Conversation

NecatiMeral
Copy link
Contributor

@NecatiMeral NecatiMeral commented Feb 20, 2020 •
edited
Loading

Resolve #2294
To support .NET-Framework based projects I've updated the corresponding projects to target netstandard2.0. The Volo.Abp.Http.Client.IdentityModel project hasn't been touched because of the IHttpContextAccessor dependency and I think it should be used by DotnetCore (Hosts) only.

Necati Meral added 2 commits February 19, 2020 10:58
Updated HttpAPi.Client projects to target netstandard2.0
73f507f 
This makes the usage of clients in traditional .netfx applications (WinForms) possible
Merge branch 'dev' of https://github.com/abpframework/abp into 2294-D…
1702ea6 
…ynamic-CSharp-Api-Clients-net-standard20
maliming
maliming reviewed Feb 20, 2020
View reviewed changes
framework/src/Volo.Abp.Http.Client/Volo.Abp.Http.Client.csproj
Comment on lines -14 to -15
<IsPackable>true</IsPackable>
<OutputType>Library</OutputType>
Copy link
Member

Choose a reason for hiding this comment

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

hi
Why remove these?

Copy link
Contributor Author

@NecatiMeral NecatiMeral Feb 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I removed these to be compliant with other netstandard projects in the framework directory.

EDIT: I looked those values up and IsPackable defaults to true and OutputType defaults to Library anyway.

@maliming maliming added this to the 2.2 milestone Feb 20, 2020
@maliming maliming self-assigned this Feb 20, 2020
@NecatiMeral
Copy link
Contributor Author

I've found a missing detail in my customization. The IdentityModelRemoteServiceHttpClientAuthenticator does in fact handle two different authentication actions:

  1. Relay the existing bearer-token in the current request
  2. Apply the IIdentityModelAuthenticationService

So the implementation details for these actions must be splitted. I'll edit my PR asap.

Necati Meral added 2 commits February 21, 2020 10:32
Created new framework package Volo.Abp.Http.Client.IdentityModel.Relay
866a5b8 
Package contains the 'pass-through' authenticator-implementation which previously was in `Volo.Abp.Http.Client.IdentityModel.IdentityModelRemoteServiceHttpClientAuthenticator`.
Since the `IdentityModelRemoteServiceHttpClientAuthenticator` implementation (without requiring `IHttpContextAccessor`) is required for basic clients (which should be able to target net-standard or netfx) this package was splitted into `Volo.Abp.Http.Client.IdentityModel.Relay` to support the known behavior of just passing the token around.

WARNING: This commit contains a breaking change for existing backend applications which use automagic C# clients!
Implemented IParameterTypeComparer to be used in `ApiDescriptionFin…
75bb5d6 
…der`

The `CrossPlatformParameterTypeComparer` handles type differences in .net-core and .net-framework by replacing the assembly names with placeholders.
This implementation works in all environments because the `CrossPlatformParameterTypeComparer` inherits the default behavior and (once) checks the current framework.
@NecatiMeral
Copy link
Contributor Author

Hi @maliming,

I've updated my PR to properly work on netfx and netcore frameworks.
Tested with a client service like ClientDemoService with identity-server and users by running a netcore backend with netcore and netfx clients.

@maliming maliming mentioned this pull request Feb 28, 2020
Closed
@maliming
Copy link
Member

hi @NecatiMeral

I merged some conflicts.
I'm not sure whether the cross-platform code conforms to the design style of abp. Because abp is all based on net core. Of course you can use this code in your project to replace the default implementation of abp. Including Volo.Abp.Http.Client.IdentityModel. Relay project.

@NecatiMeral
Copy link
Contributor Author

hi @maliming

Thanks for your efforts. Our preferred solution in the long-term view would be to use the upstream modules / packages instead of customizing every bit just for our needs. I think there are multiple developers who are requiring netstandard-clients for different scenarios (mobile, winforms, ...).

I know that abp was created because of the huge changes in netcore3.1. For the backend aspect it's totally fine, but maybe abp should consider this compatibility for clients. the effort to maintain netstandard compatibility on client-side is pretty low.

We can't use abp if the built-in-clients cannot be used in netstandard-projects because we don't want to manually generate our clients for each api version (with AutoRest for example).

@maliming maliming requested a review from hikalkan March 2, 2020 02:10
@hikalkan
Copy link
Member

hikalkan commented Mar 2, 2020

I simplified this implementation: #2951
@NecatiMeral can you check if it has any problem?
I removed some classes. I especially didn't like the EnvironmentHelper which is a little ugly.
What do you think?

@hikalkan
Copy link
Member

hikalkan commented Mar 2, 2020

I also didn't find the .Relay project naming useful. I will change it as .Web.

@NecatiMeral
Copy link
Contributor Author

Thanks for your feedback. I'll review the changes tomorrow.

@hikalkan hikalkan mentioned this pull request Mar 3, 2020
Closed
@hikalkan hikalkan merged commit 4929986 into abpframework:dev Mar 3, 2020
@NecatiMeral
Copy link
Contributor Author

hi @hikalkan,

thank you for the review / updates. Looks good to me!

@NecatiMeral NecatiMeral deleted the 2294-Dynamic-CSharp-Api-Clients-net-standard20 branch March 3, 2020 08:52
@realLiangshiwei realLiangshiwei mentioned this pull request May 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maliming maliming maliming left review comments

@hikalkan hikalkan Awaiting requested review from hikalkan

Assignees

@maliming maliming

Labels
effort-0.5
Projects
None yet
Milestone
2.2
Development

Successfully merging this pull request may close these issues.

Volo.Abp.Http.Client.IdentityModel must specify Microsoft.NET.Sdk.Web sdk ?
3 participants
@NecatiMeral @maliming @hikalkan