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.

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

Extending an ABP controller #7721

Closed
sonicmouse opened this issue Feb 14, 2021 · 13 comments · Fixed by #7766
Closed

Extending an ABP controller #7721

sonicmouse opened this issue Feb 14, 2021 · 13 comments · Fixed by #7766

Comments

@sonicmouse
Copy link

I know you can override an ABP controller: https://docs.abp.io/en/abp/latest/Customizing-Application-Modules-Overriding-Services#example-overriding-a-controller

But, how would you add an endpoint to the overridden controller? I have tried adding an endpoint and I can't hit it with Postman and it doesn't show up in swagger. For example:

[Dependency(ReplaceServices = true)]
[ExposeServices(typeof(IdentityUserController))]
public class MyIdentityUserController : IdentityUserController
{
    public MyIdentityUserController(IIdentityUserAppService userAppService) : base(userAppService)
    {
    }

    public override Task<PagedResultDto<IdentityUserDto>> GetListAsync(GetIdentityUsersInput input)
    {
        // this works fine and it gets hit
        return base.GetListAsync(input);
    }

    [HttpGet, Route("mymethod")]
    public Task<string> MyMethod()
    {
        return Task.FromResult("Doesn't work");
    }
}

How can I accomplish this?

@olicooper
Copy link
Contributor

Duplicate: #7698

@sonicmouse
Copy link
Author

sonicmouse commented Feb 14, 2021

Duplicate: #7698

That sure isn't a duplicate. The solution provided doesn't solve the issue. The solution provided implements a visual kludge in swagger by using the same Route property as IdentityUserController. It doesn't extend the service controller.

[RemoteService(Name = IdentityRemoteServiceConsts.RemoteServiceName)]
[Area("identity")]
[ControllerName("User")]
[Route("api/identity/users")]
[ExposeServices(typeof(MyIdentityUserController))]
public class MyIdentityUserController : AbpController, IApplicationService, IRemoteService
{
    [HttpGet("my-method")]
    public Task<string> MyMethod()
    {
        return Task.FromResult("Works");
    }
}

How is that extending IdentityUserController in any way? How does that inherit IdentityUserController methods, properties and base logic?

As a matter of fact, i don't see IdentityUserController anywhere in that code.

@olicooper
Copy link
Contributor

Both questions ask about extending an existing abp controller to add a new method. The only difference is that the other question also says they want to override an existing method. I would consider this a duplicate.

The only thing missing from that code to allow you to 'extend' functionality is a constructor with the AppService for you to access the available app service methods. this will appear the same to any user of the system.

public MyIdentityUserController(IIdentityUserAppService userAppService)
{
}

public Task<??> MyAdditionalMethod(SomeInput input)
{
    return userAppService.DoSomething(input);
}

I wouldn't call it 'kludge', it's not like using magic strings, or having 100 parameters for a method. I don't believe it has any effect on performance, nor is it a hack. I also believe it does solve the issue of extending the available API methods. May I ask in what tangible way this approach is not feasible for your needs? I am open to a better solution but I don't believe one exists - though I would be interested if one does 👍 😄 Please also refer to my latest comment on the other issue: #7698 (comment)

@sonicmouse
Copy link
Author

sonicmouse commented Feb 14, 2021

What if i wanted to IIdentityUserAppService in conjunction with public virtual Task<IdentityUserDto> FindByEmailAsync(string email) that IdentityUserController implements?

How do I do that?

What I am saying is: you are simply creating a new controller. That's it. Your fix doesn't have anything to with inheritance, which is what we want to accomplish. We want to inherit from IdentityUserController not create a new class and throw it in the same category as IdentityUserController.

It seems ABP breaks a very basic inheritance pattern that OOP gives us. It seems. Which I highly doubt. So there is something we must be missing, which is why I opened an issue.

I don't know how else explain it.

@olicooper
Copy link
Contributor

Yep okay no problem, I will see if it is possible to do it the way you describe when I am next able to. I am curious too. Until then I think we will have to wait for a reply from ABP on the matter. I still think it is a duplicate though 😄

@sonicmouse
Copy link
Author

The issue you linked talked about swagger not working -- This isn't a swagger issue since you can't hit the new endpoint via Postman. I could just remove the mention of swagger in the question and the issue is still relevant.

@Ultre00
Copy link

Ultre00 commented Feb 14, 2021

The linked issue started as a different issue but in the end turned out to be the exact same problem as you described in this ticket. I started the GitHub issue before I created that stack overflow issue. Your question sums up the exact same problem I am facing as of this moment. Also looking for a clean way to extend an existing controller with one or more functions and still being able to override in the same extended class.

@maliming
Copy link
Member

hi @sonicmouse

  • Your ABP Framework version.
  • Steps needed to reproduce the problem.

@Ultre00
Copy link

Ultre00 commented Feb 15, 2021

@maliming it is with version 3.3.2. I created a project to reproduce the issue. Created an empty project with the cli from 3.1.2 (3.3.2 didn't work anymore). Then I manually upgraded to 3.3.2 (also because abp update -v 3.3.2 for some reason isn't working anymore today).

https://github.com/Ultre00/AbpExtendingControllers/blob/11c7cb832cfe80db9a91945786a36de528d51396/src/AbpExtendingControllers.Web/Controllers/MyAccountAcontroller.cs#L22

That is the only class I added. When you call the register from postman the overridden function gets called. The my-function endpoint always returns a 404. Also not visible in swagger

@maliming
Copy link
Member

@Ultre00

Have you seen this? #5269 (comment)

@Ultre00
Copy link

Ultre00 commented Feb 16, 2021

@maliming yes I have seen it. The overriding part isn't the problem. That does actually work. It is adding a new method that doesn't work

@olicooper
Copy link
Contributor

I have had a look at the source code and I can see that derived controllers are removed when the application starts up, which will likely stop the additional actions in the derived controllers being registered.

protected virtual void RemoveDuplicateControllers(ApplicationModel application)
{
var derivedControllerModels = new List<ControllerModel>();
foreach (var controllerModel in application.Controllers)
{
if (!controllerModel.ControllerType.IsDefined(typeof(ExposeServicesAttribute), false))
{
continue;
}
if (Options.IgnoredControllersOnModelExclusion.Contains(controllerModel.ControllerType))
{
continue;
}
var baseControllerTypes = controllerModel.ControllerType
.GetBaseClasses(typeof(Controller), includeObject: false)
.Where(t => !t.IsAbstract)
.ToArray();
if (baseControllerTypes.Length > 0)
{
derivedControllerModels.Add(controllerModel);
Logger.LogInformation($"Removing the controller {controllerModel.ControllerType.AssemblyQualifiedName} from the application model since it replaces the controller(s): {baseControllerTypes.Select(c => c.AssemblyQualifiedName).JoinAsString(", ")}");
}
}
application.Controllers.RemoveAll(derivedControllerModels);
}

The log:

[INF] Removing the controller MyApp.Controllers.MyIdentityUserController, MyApp.HttpApi.Host, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null from the application model since it replaces the controller(s): Volo.Abp.Identity.IdentityUserController, Volo.Abp.Identity.HttpApi, Version=3.3.2.0, Culture=neutral, PublicKeyToken=null

Not sure how this would be fixed though.

@maliming
Copy link
Member

See #7766

@maliming maliming removed their assignment Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants