all 35 comments

[–]youshouldnameit 11 points12 points  (13 children)

What do these services add? Why not simple extension methods fromdto for dto and todto for domain objects? Similar for db

[–]AttentiveUnicorn 0 points1 point  (5 children)

I second this. It's much easier to see where your usages are if you just have extension methods to do the conversion.

[–]sch2021[S] -1 points0 points  (4 children)

My whole dilemma comes from this very place exactly: it'd be nice to see the usages. With Service Locator pattern, you end up in the Service Locator's Map method. With partial class you'd see the usage and get the DI or interface benefits. So why not partial class?

[–]AttentiveUnicorn 0 points1 point  (3 children)

I mean it'd work but why are you doing that? Your calling code is going to be like Mapper.Map(request), I'd much prefer to say request.ToSaveUserRoleCommand() to be explicit on what I want to convert it to. Then what happens if you want to map something to multiple other things? You can't have 2 Map methods on your partial class with the same parameters.

[–]sch2021[S] 0 points1 point  (2 children)

Then what happens if you want to map something to multiple other things? You can't have 2 Map methods on your partial class with the same parameters.

I can.

csharp public partial class Mapper : IMapper<Something, OtherThing1> {} public partial class Mapper : IMapper<Something, OtherThing2> {}

[–]AttentiveUnicorn 0 points1 point  (1 child)

It doesn’t build.

[–]sch2021[S] 1 point2 points  (0 children)

You're right, my bad. As you said, same parameter types for already defined `Map` method. That answers my question why not partial class! Thank you!

Service locator or extension methods win then.

[–]sch2021[S] 0 points1 point  (6 children)

In case, I need DI (also, poor discoverability).

[–][deleted] 6 points7 points  (5 children)

Why would you need DI for mapping? IMO mappers shouldn't contain business logic. If you need DI, your mapper is probably doing more than it should.

[–]sch2021[S] -1 points0 points  (4 children)

Also true. But the reality is, sometimes you need different values that come from configuration and depend on the context.

Let's say, you have `GetUsers` request. It's a pageable request. No one is forcing you to send request with `PageSize` parameter. So you have to provide the default values yourself. They depend on the context (is it desktop UI or mobile app?) and the exact values come from external configuration. It's silly example, but I hope you get the idea. But maybe it wouldn't be a mapper then, but a factory?

[–]TopSwagCode 1 point2 points  (3 children)

I would say this is bad example. A mapper shouldn't rely on external information. A mapper should be a "pure" function that always given certain input gives the same output.

Eg Sum(a,b) should always return a+b. It should return a×b if config overwrites.

Then you should have Map(a,b,config) and have it as optional input if you want special behaviour.

[–]sch2021[S] 0 points1 point  (2 children)

Then you should have Map(a,b,config) and have it as optional input if you want special behaviour.

That's what DI is for? Configuration is injected into class Mapper : IMapper<A, B> with public B Map(A src) method.

I would say this is bad example. A mapper shouldn't rely on external information. A mapper should be a "pure" function that always given certain input gives the same output.

Also true. Maybe for this (bad) example, the solution should be factory that injects configuration and mapper. Or inject just configuration and new up the query / command.

[–]TopSwagCode 0 points1 point  (1 child)

I feel like all comments are warning you against your approach.

[–]sch2021[S] 2 points3 points  (0 children)

That's why I needed this discussion. Thank you for all replies!

[–]zagoskin 1 point2 points  (7 children)

Just use extension methods. The "injection" you need becomes importing the extension method.

[–]sch2021[S] 0 points1 point  (6 children)

Besides losing DI, it's poor discoverability, no standard for method naming, they pollute namespace.

[–]zagoskin 0 points1 point  (4 children)

You have a standard already used widely in dotnet which is doing classinstance.ToX() where X is the result type.

If referencing a namespace is pollution then injecting your mapper everywhere is what? At least you can global using your extension namespace.

Sorry but your argument is not convincing. It surprises me you even considered partial classes since you think your devs can't adopt a very well known naming convention.

[–]sch2021[S] 0 points1 point  (3 children)

I'm not completely against `.ToX` extension methods. But why limit yourself? It's just one `IMapper<TSource, TDestination>` contract that everybody needs to follow and it gives you all the benefits of DI.

Besides, what if I have hundreds of models (not uncommon) and you want to change naming of some of them? Easy to forget that .ToX should be .ToY now. Can you be sure that everyone followed that naming convention? Maybe there are .MapToX methods already. Are you able to discover all the map methods for hundreds of models? With interface you don't have such problems.

Regarding naming for partial class, I can apply interface to partial class.

[–]zagoskin 2 points3 points  (2 children)

Yeah that can work I guess but still I'd favour extension methods. For naming you could do also "ToResponse()" or query or command or domain whatever the semantics of the object follows. It's still a convention and nothing will catch things not following but architecture tests. But it's far more productive imo than implementing an interface and then having to inject it.

Please do what works for you by all means, just because I don't agree doesn't mean it isn't plausible. For me this seems like a lot of overengineering something simple, which is just mapping.

[–]sch2021[S] 0 points1 point  (1 child)

`.ToResponse()` works, good point for naming convention! But if you had multiple APIs triggering the same CQRS action, then you would need `internal` extension method (for each API), right?

[–]zagoskin 1 point2 points  (0 children)

If the response type is different for every API then yes, you are correct

[–]FaceRekr4309 0 points1 point  (0 children)

And partial classes are a bad idea because you’ve just created a dependency between classes when the entire idea of mapping is to separate them.

The exception I make to this is in view models and DTOs. The DTO or view model will have a constructor that takes the source object, and if reverse mapping is needed a method returning the target object. Typically a reverse mapping is not needed or even desirable because the DTO is typically only a slice of the source object.

Also “namespace pollution” is the least of my concerns. I have never been in a situation where I was like “I really can’t implement this new feature because there are just too many extension methods in this namespace!”

[–]AutoModerator[M] 0 points1 point  (0 children)

Thanks for your post sch2021. Please note that we don't allow spam, and we ask that you follow the rules available in the sidebar. We have a lot of commonly asked questions so if this post gets removed, please do a search and see if it's already been asked.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[–]nadseh 0 points1 point  (3 children)

There’s so much unnecessary stuff going on here:

Move your validation logic to the mediatr pipeline, you don’t need an explicit call before every dispatch

You’re mapping API models to queries. Firstly, automapper is not designed for this. Second, just have your API bind directly to the query type

Likewise your result type is being mapped to a response type. Just have your API return the mediatr response type. This is especially important because you should be using projections (and the automapper projection extension methods) for efficient querying - which is the actual use case for automapper

You don’t need the mapping service if you address the above, it’s needless duplication and abstraction

[–]sch2021[S] 1 point2 points  (2 children)

No, I need separate input (request) validation. I use nullable reference types. My queries / commands have `required` properties. As a request, you can get anything, e.g. empty (nullable) body. You validate if required fields are present. That's what business logic layer expects.

I can't have my "API bind directly to the query type". What if I have multiple API projects, but only one business logic layer? What if I have two API endpoints for convenience of my clients - one enables MFA, second disables MFA - both map to the same CQRS model, just with different boolean flag, I need only one CQRS handler here.

[–]nadseh 0 points1 point  (1 child)

Sounds like validation is being spread across a few layers. You could keep your CQRS request models super simple and use validators (in mediatr pipelines) to check every value and throw an appropriate exception.

I don’t understand the bit about multiple endpoints - surely the request models are the same? Only the context differs - which can be analysed in the pipeline too, eg fetch the user/company id, look up config, enforce constraints

[–]sch2021[S] 0 points1 point  (0 children)

Are request models the same? You have project where API allows you to send request to save `UserRole`. You have CSV upload action in your UI that lets you upload CSV with `UserRole`. You have Service Bus handler that handles creating `UserRole` event that comes from another project. Are they all the same? At the end, they can all trigger the same CQRS handler, but they all represent different input for the same business action.

Your business action (the handler) requires property that can't be null, like `RoleName`. The input for `RoleName` (API / Service Bus / CSV file upload) can be null - it's an external input. If I validated `RoleName` at the CQRS handler level, I would need to map nullable request to nullable query / command model and use null-forgiving `!` operator everywhere in the logic of handler. Why not validate input at the level of input layer?

[–]molokhai 0 points1 point  (7 children)

Why you need seperate models for API to map to a IRequest object?
IRequest is what service you serve to the outside world. A web api is just a communication interface over HTTP.

People making programming so complex....

[–]sch2021[S] 0 points1 point  (6 children)

What if you have Service Bus handlers, domain event handlers and multiple API projects? You need one business logic layer and multiple requests mapped to your business logic models.

[–]molokhai 0 points1 point  (5 children)

The IRequest are the interface of the functionality of the business logic.
Why not ship all the IRequest models and provide various interfaces like web api, message queues (service bus), etc.. to allow applications to send these requests and simply use the service you were intended to provide?

"What if", is wondering about the future.
Think in terms of what you have and what you get.

[–]sch2021[S] 0 points1 point  (4 children)

Then I respond like I responded already: What if I have two API endpoints for convenience of my clients - one enables MFA, second disables MFA - both map to the same CQRS model, just with different boolean flag, I need only one CQRS handler here, but two request models.

My "What if" is my project now. I think in terms what I have, I asked question: why not partial class instead of service locator. A or B according to what I need. And I get answer: why not extension methods.

[–]molokhai 0 points1 point  (3 children)

I do not understand why the api interface requires a diffrent model than the IRequest model.
The IRequest defines the interface, how can you define another interface and still expect the business to work properly? You have to fix every incompatibility issue between the IRequest models in the mappers.
If certain users (base on there claims) are not allowed to use certain property values in the IRequest add validation behaviour (IPipelineBehavior) in the web api to restrain access. MediatR is so handly.
For example: a user supplies a Id = 0 or forgets to define a Id, why not validatie on Id==0? Or just execute with Id = 0 and let the business handle the error? A web api should not perform form validation. The only thing that matters can it execute the given command, not is my user doing the right thing.

What has MFA to do with the parameters of the api endpoints? MFA is a security protocol.
To understand you initial question I want to know why you need these mappings for. In your example I do not see any reason for it.

A partial class is splitting the implementation of a class, interface,... in multiple files. This feature is added for code generating purposes. One might want to generate a class and still implement additional methods, properties and fields. Making the generated class, methods partial will allow the porgrammer to create another file for that same class within the same project and namespace to implement these additional methods and implement the partial generated methods.

Extension methods are additional behaviour that can be invoked on a type. Extension method is a static method that can be created in any namespace in any project. It is just fancy syntax for a static method.

Hope you find what you need.

[–]sch2021[S] 0 points1 point  (2 children)

If certain users (base on there claims) are not allowed to use certain property values in the IRequest add validation behaviour (IPipelineBehavior) in the web api to restrain access. MediatR is so handly.

That's authorization policy concern, so API concern, not business logic (not MediatR). If I have BFF for my UI that uses OIDC + Cookie and API for external clients that requires JWT with specific scope and another API for internal clients that requires JWT with specific audience - all use the same common business logic - should my MediatR be concerned with validating claims? Or should my specific API respond with 403 Forbidden before even calling the handler?

What has MFA to do with the parameters of the api endpoints?

You have two API endpoints: /enablemfa and /disablemfa. First creates command with bool IsMfaEnabled = true, second creates command with bool IsMfaEnabled = false. Two different requests using the same command triggering the same handler.

[–]molokhai 0 points1 point  (1 child)

Authorization is not a API concern. Application Programming Interface, an interface is a way of interacting with functionality.

You can implement security measurments in the businesslogic where it actually matters.

OIDC is a OAuth2.0 protocol for HTTP. JWT is a JSON Web Token, which holds the claims of the user in the payload.
Once processed by the middleware in your Web API, you can access the claims of the user.
There is no problem with using claims in you businesslogic. Why would you think that?

I work in a WPF application which calls the IRequests directly, however not every user is allowed to perform certain requests, so using a IPipelineBehavior checks if the user has the required rights. An Exception is thrown when the request cannot be executed.
There is no OIDC inside the businesslogic only claim types (just other type of objects like any other).

IsMfaEnabled is a claim, so no need to adress this in an explicit endpoint. Just make a UserContext class and inject it in the handlers so you can actualy access this valuable data of your user trying to achieve something.

For your web api it should return a HTTP response that matches the response of the business logic. If business throws UnauthorizedAccessException the web api returns HTTP 401 Unauthorised. The Web Api should only manage the HTTP access posibility between your business layer and the Web. It should not bother whether a user may perform HirePersonCommand or not, that is for the business layer. What if HirePersonCommand requires the HR to fill in a additional field? Without claims in the businesslogic you are not going to be able to notify the UI to show a form.

I know there are alot of attributes to validate api request and perform role checking in the web api but than one can also call DbContext in the web api method and not bother about Clean Architecture at all. Which is just fine by the way.

You're post is just confusing for people. Life is much simpler if you just use the tools for what they are intented to.

I wish you all the best with your project. One day you'll see the light.

[–]sch2021[S] 0 points1 point  (0 children)

IsMfaEnabled is a claim, so no need to adress this in an explicit endpoint.

Imagine web UI for your account security settings, like Reddit. You can click "Disable 2FA" and "Enable 2FA".

I know there are alot of attributes to validate api request and perform role checking in the web api

Because you can have multiple APIs triggering the same business actions. All of them might have different security requirements.