Web API Design

[jadjare]

I was just trying to read up on Web API conventions, as I want to implement #24, and was wondering what (if any) the convention would be for a simple GET query of ItemExistsForId(Guid id) type method.

This led me to this post... https://docs.asp.net/en/latest/tutorials/first-web-api.html

There's a few things in this post that look like we might want to adapt, e.g...

• Action methods always returning an IActionResult
• Action methods can return the built in action results, such as, return NotFound() to represent a 404 errors
• Models appear to be wrapped up in the ObjectResult container, or use the CreateAtRoute method
• Action methods also seem to have "natural" names rather than names matching the Http Verb

What do you think?

Also, is it correct to just add a [HttpGet] public IActionResult ItemExistsForId(Guid id) method to allow clients to check a product exists?

[johncollinson2001]

Ello,

Good reading :)

Defo use all the examples in the post. IActionResult is a good one allowing the use of the built in results.

Not seen the object result before but if that's the suggested result in the docs the gotta be worth doing.

Also natural names defo seems like a good idea.

I guess the only comment would be on having a method for item exists for id, I'd expect clients to just call getforid and check the response to see if an item exists (or check for a not found response)?

[jadjare]

Ok, I'll look at refactoring in my branch today... See how it looks. Will try a pull request when done.

As for the ItemExists method, I'm not so sure either! The idea was to allow a client to check before taking an action, for example deleting a product without having to get the entire item first. I now think it's probably a security risk.

Anyway, I'll definitely leave it off for now. I want to explore further what happens when a client tries to delete or update a product that doesn't exist. It's really about keeping exceptions exceptional and allowing clients to ask before taking actions that might fail. What I'm not so sure about is how all of that manifests itself when using a web API. But a quick play with the console app will tell me.

[jadjare]

Problem Statement

I've been looking at the refactoring and this has thrown up a few questions.... primarily on validation, command handlers, and returning model state errors.

In essence, the command handlers are currently agnostic to the web api and controllers, this makes handling validation issues difficult. At the moment I've put up 3 possible solutions, and I'm sure there's more, these 3 options are essentially seen in the ItemController as...

        //TODO: Decide V1 - Using command handler agnostic to service and web api
        [HttpPut("{id}")]
        public void Put(Guid id, [FromBody]UpdateItemModel model)
        {
            var command = new UpdateItemCommand(id, model.Name, model.Description);
            _updateItemHandler.Execute(command);
        }

        //TODO: Decide V2 - Using action handler, inherently tied to web api, uses model to cut out middleman and ensure model state errors reflect model properties
        [HttpPut("PutV2/{id}")]
        public IActionResult PutV2(Guid id, [FromBody]UpdateItemModel model)
        {
            return _updateItemHandlerV2.Execute(model, this);
        }

        //TODO: Decide V3 - Does the work directly in the body of the Action Method, forgoing commands and handlers with the downside of having chunky action methods
        [HttpPut("PutV3/{id}")]
        public IActionResult PutV3(Guid id, [FromBody]UpdateItemModel model)
        {
            if (model == null) return BadRequest();

            //var maybeItem = _unitOfWork.ItemRepository.GetForId(id);
            var maybeItem = _itemRepository.GetForId(id);

            if (!maybeItem.HasValue()) return NotFound();

            var item = maybeItem.Single();

            var canChangeName = item.CanChangeName(model.Name);
            var canChangeDescription = item.CanChangeDescription(model.Description);
            if (!canChangeName || !canChangeDescription)
            {
                ModelState.AddModelError(nameof(model.Name), canChangeName.ErrorMessage);
                ModelState.AddModelError(nameof(model.Description), canChangeDescription.ErrorMessage);
                return BadRequest(ModelState);
            }

            item.ChangeName(model.Name);
            item.ChangeDescription(model.Description);

            //_unitOfWork.Commit()

            return NoContent();

        }

Options

V1 - As-Is representation

With this implementation it's not possible to communicate errors back to the client. At present both validation issues and system errors generate an Exception and currently just result in a generic 500 error.

V2 - Action Handlers not Command Handlers

This implementation (very crudely done) essentially changes the CommandHandlers into ActionHandlers and insists the ControllerBase instance is passed to the Execute method in it's context parameter (I couldn't figure out a more elegant way to do this!). It also does away with the Command Object and simply uses the model, this is primarily to ensure that model state errors relate directly to the model, else either convention or mapping would be needed between a command and model, in addition it reduces the amount of code duplication between a model and command. @johncollinson2001 I remember you felt the distinction between a model and command was necessary, I assume this was because the UpdateCommand had the Id field whereas the UpdateModel didn't - was this why? From looking at examples, it seems common to have the Id both in the model and in the route.

The V2 implementation is my current favourite, but more than open to suggestions.

V3 - No Handlers

The V3 implementation just does away with handlers all together... this lets the action method interact directly with the ControllerBase and associated ModelState. It's advantage is that it requires less wiring up, however, it has the downside of not keeping the handling of actions nicely organised.

Vx - Your Suggestions

The Prototyping Code

I've pushed the changes into the "develop-#1" branch. Apologies I forgot to rebranch when I started to make the changes. The code is largely hacked in at the moment, as I wanted to explore options rather than organise the code neatly.

Here's the other highlights and corresponding TODOs!

Added CanChangeName / Description methods to the Item Class (note the TODO around localisation)

        public ValidationResult CanChangeName(string name)
        {
            //TODO: Consider localisation concerns and general problem that the domain should not be concerned about localisation - will probably need to use ErrorCodes or Keys (e.g. resource file key)
            return new ValidationResult(string.IsNullOrWhiteSpace(name) == false,
                "name must not be null, empty or whitespace");
        }

Also note, I'm beginning to increasingly think that Name and Description themselves should be bound into a single ValueObject - but that's another topic to discuss later!

Created a simple ValidationResult class. This is basically my own creation, so don't expect rocket science here! but enough to solve the problem! It's bunged at the bottom of the Item class hence the TODO.

    //TODO: This is just here as a temporary measure whilst working out the best route
    public class ValidationResult
    {
        public ValidationResult(bool isValid)
        {
            IsValid = isValid;
        }

        public ValidationResult(bool isValid, string errorMessage) : this(isValid)
        {
            if(errorMessage==null) throw new ArgumentNullException(nameof(errorMessage));
            if(!isValid) ErrorMessage = errorMessage;
        }

        public bool IsValid { get; }

        public string ErrorMessage { get; } = string.Empty;

        public static implicit operator bool(ValidationResult validationResult)
        {
            return validationResult.IsValid;
        }
    }

What Next?

I'm no expert is this area at all, so... _What do you think? Any other ideas, want to discuss in person (happy to do at mine), want to play with concepts yourselves.__

Postman

FYI - As per the first link below, I downloaded the Postman addin for Chrome... I found it quite helpful for testing out the API.

Interesting articles read during the exploration of ideas

https://docs.asp.net/en/latest/tutorials/first-web-api.html https://www.asp.net/mvc/overview/older-versions-1/models-data/validating-with-a-service-layer-cs http://stackoverflow.com/questions/10097828/localizing-validation-messages-from-domain-objects-entities

Example of CanDoSomething style method at circa 4:50 - (note: Vladimir goes through it more thoroughly somewhere else in the course, after several scan throughs though I can't find it!) https://app.pluralsight.com/player?course=domain-driven-design-in-practice&author=vladimir-khorikov&name=domain-driven-design-in-practice-m6&clip=9&mode=live

[johncollinson2001]

Hi Joe,

Can the handlers either raise an exception when a validation error occurs, or return some kind of result wrapper which has the status of the command and any errors inside?

That way the controller can be concerned only with executing the command and transforming the result (or exception) into properties on the model state.

I don't think the service should be concerned with much else. Also really feels like the commands shouldn't know about the service.

So...

Handler:

If (!CanChangeName) result.errors.add(new ValidationError("couldn't change name"));

Controller: Var result = handler.Execute(); If(result.Errors.Any()) // Update the model state errors

All pseudo code there! But something like that should work?

[johncollinson2001]

Btw git nugget for the day... If you realise youve made changes on the wrong branch, stash them switch branch, pop the stash onto your new branch:

git stash save "my uncommitted changes"

git checkout new-branch

git stash pop stash@{0}

[dibley1973]

I like what @jadjare 's V3 gives us, but that does mean a lot of code in each controller action... I guess the modification from @johncollinson2001 allows the validation logic to be applied in the handler with the controller action just checking for validation errors and applying to the model, or returning back the the caller.

So for me it's V3 but with some of the logic extracted to the handler? Unless there is another way still to bubble up!

[dibley1973]

@jadjare - Is this code checked into any of your branches for review?

[jadjare]

Yes in the develop-#1 branch

[jadjare]

I'm still erring on either option 2 or 3... This is primarily because I think that the Command Handlers are part of the Service and I'm not sure there's much advantage in keeping them agnostic to the Web API technology base chosen for the Application Services layer.

Hopefully will get a chance to play with a few more ideas later, especially to get rid of the controller injection into the Execute method.

[jadjare]

I've pushed up some tweaks to the v2 option to the "develop-#1" branch.

Primarily I've removed the need to inject the Controller into the Execute method and cleaned up the code a bit. Here's the outtake below

Here's the Handler code

        public IActionResult Execute(UpdateItemModel model)
        {
            if (model == null) throw new ArgumentNullException(nameof(model));

            using (var unitOfWork = _unitOfWorkFactory.CreateUnitOfWork())
            {
                var maybeItem = unitOfWork.ItemRepository.GetForId(model.Id);
                if (maybeItem.HasValue()==false) return new NotFoundResult();

                var item = maybeItem.Single();

                var canChangeName = item.CanChangeName(model.Name);
                var canChangeDescription = item.CanChangeDescription(model.Description);

                if (canChangeName && canChangeDescription)
                {
                    item.ChangeName(model.Name);
                    item.ChangeDescription(model.Description);

                    unitOfWork.Complete();
                    return new OkResult();
                }

                var errors = new ModelStateDictionary();
                if (!canChangeName) errors.AddModelError(nameof(model.Name), canChangeName.ErrorMessage);
                if (!canChangeDescription) errors.AddModelError(nameof(model.Description), canChangeDescription.ErrorMessage);
                return new BadRequestObjectResult(errors);
            }
        }

And here's the controller action method

        [HttpPut("PutV2/{id}")]
        public IActionResult PutV2(Guid id, [FromBody]UpdateItemModel model)
        {
            return _updateItemHandlerV2.Execute(model);
        }

The above is a little cleaner than my previous incarnation of option 2. At this point in time, I'm not convinced there's any advantage to be gained by abstracting out the IActionResult and ModelStateDictionary. I guess the question is does it matter if the handlers are fundamentally tied into the web api framework?

p.s. @johncollinson2001 , your pseudo code option of returning ValidationErrors (rather than model state errors) from the handler at first looks attractive. However, there are a few stumbling blocks to get over.

• Different errors will need to return different http responses, e.g. NotFoundResponse when an Item cannot be found, BadRequest when validation fails and OKResult after all has succeeded. From reading up I think these I believe are the standard responses expected. This means that code will need to be added to the Action method to somehow interpret the difference between an Item cannot be found and validation errors, so the right HTTP response can be sent back.
• The Validation Errors should be tied back to the relevant model property. I guess this will be easy if the command and the model are one in the same and therefore the action method and handler use the same class. But if they use different classes we'd have to create a mapping between the two or rely on convention (i.e. property names being identical in each) in order to record the validation error against the right model property.

It's these two reasons that make me doubt whether it's worth the effort of keeping the command handler agnostic to the web api framework.

[johncollinson2001]

I feel strongly that command handlers should not know about web api classes such as model state.

What happens when someone turns round and days they want the application exposed via wcf XML based services, or via some kind of messaging system? We really shouldn't bake it things like model state into the commands, it just feels wrong to me.

A simple mapping in the controller between the apps concept of error types and the different http responses is the way to go.

I get the whole keep it simple, yagni, etc etc. But this feels quite a fundamental bleeding of technology specific concerns into the commands which just feels wrong to me.

[jadjare]

OK, so I had another look again taking into account the various bits of feedback above and my thoughts.

I'm keen to avoid having commands that fail swallowing up the failures and returning them as soft errors rather than exceptions. I'm also keen to avoid calling methods needing to adopt a try, catch block in order to handle potentially expected situations and then sift exceptions into those that are validation issues vs genuine exceptions. Also taking into account @johncollinson2001 's points about stopping technology concerns bleeding into the commands, I've come up with the following outline of a concept:

CommandHandlers can utilise a ICommandHandlerWithPreconditionCheck interface. This interface enforces the implementation of a PreconditionsMet(command) method. This method can then check any necessary preconditions are met. The caller can then invoke this method, prior to Executing the command.

Here are the excerpts of code that illustrate it's usage... It's not perfect but I hope it's a good start:

Command Handler with preconditions check interface

    public interface ICommandHandlerWithPreconditionCheck<in TCommand>: ICommandHandler<TCommand> where TCommand: Command
    {
        PreconditionCheckResult PreconditionsMet(TCommand command);
    }

PreconditionCheckResult and PreconditionFailure classes

public class PreconditionCheckResult
    {
        private readonly List<PreconditionFailure> _failures = new List<PreconditionFailure>();

        public IReadOnlyCollection<PreconditionFailure> Failures
        {
            get { return _failures.AsReadOnly(); }
        }

        public void AddFailure(PreconditionFailure failure)
        {
            _failures.Add(failure);
        }

        public void AddFailure(string propertyName, string failureMessage)
        {
            var  failure= new PreconditionFailure(propertyName,failureMessage);
            _failures.Add(failure);
        }

        public static implicit operator bool(PreconditionCheckResult preconditionCheckResult)
        {
            return !preconditionCheckResult.Failures.Any();
        }
    }

    public class PreconditionFailure
    {
        public PreconditionFailure(string property, string failureMessage)
        {
            if(string.IsNullOrWhiteSpace(property)) throw new ArgumentNullException(nameof(property));
            if (failureMessage == null) throw new ArgumentNullException(nameof(failureMessage));

            Property = property;
            FailureMessage = failureMessage;
        }

        public string Property { get; }
        public string FailureMessage { get; }
    }

Extension Method to convert PreconditionFailures to ModelState errors, this is quite crude for now

    public static class PreconditionCheckResultsExtensions
    {
        public static ModelStateDictionary AsModelState (this IReadOnlyCollection<PreconditionFailure> instance)
        {
            var modelStateErrors = new ModelStateDictionary();
            foreach (var preconditionFailure in instance)
            {
                modelStateErrors.AddModelError(preconditionFailure.Property,preconditionFailure.FailureMessage);
            }

            return modelStateErrors;
        }
    }

Example Controller Action Method Utilising the PreconditionsMet query (note, also added a modelstate check before beginning the command and put DataAnnotations on the model)

        [HttpPut("PutV4/{id}")]
        public IActionResult PutV4(Guid id, [FromBody]UpdateItemModel model)
        {
            if (ModelState.IsValid == false) return BadRequest(ModelState);

            var command = new UpdateItemCommand(id, model.Name, model.Description);

            var preconditionsMet = _updateItemHandler.PreconditionsMet(command);
            if (!preconditionsMet) return BadRequest(preconditionsMet.Failures.AsModelState());

            _updateItemHandler.Execute(command);
            return NoContent();

        }

I'll get some invites out shortly for a get together. But I hope this is now along the right lines. Feels better to me!

[dibley1973]

I did like the version @jadjare offered 4 days ago (no perma link available to link to) but then the comments @johncollinson2001 made also felt very valid too.

Looking at the revised structure that @jadjare has laid out above, I feel that still keeps the controller action methods clean and easy to read without bleeding of technology concerns which @johncollinson2001 pointed out.

So I am happy with this latest proposition.

[jadjare]

I should add that I don't see the Preconditions Check as mandatory on all command handlers, only those that have complex validation or validation that requires infrastructure checks, e.g. name is already in use. (http://gorodinski.com/blog/2012/05/19/validation-in-domain-driven-design-ddd/)

Although I've gone around the houses on it, there's a reasonable argument that placing the Data Annotations on the UpdateItemModel class are good enough to ensure a high probability of success (http://disq.us/p/x0n19e), thus the precondition checks in this instance aren't really necessary!!

I've just noticed, that I didn't post the UpdateItemHandler code above, so here it is below. You'll see that the only precondition that isn't really predictable and relies on infrastructure is whether the Id exists or not. I'm open for debate on whether this is significant enough for a pre-condition check or just should be assumed to be correct

UpdateItemHandler code (note Execute method is basically unchanged)

public void Execute(UpdateItemCommand command)
        {
            if (command == null)
                throw new ArgumentNullException(nameof(command));

            using (IItemManagementUnitOfWork unitOfWork = _unitOfWorkFactory.CreateUnitOfWork())
            {
                var result = unitOfWork.ItemRepository.GetForId(command.Id);

                // Ensure product exists to update
                if (!result.HasValue())
                    throw new InvalidOperationException(string.Format(ExceptionMessages.ProductNotFound, command.Id));

                var item = result.Single();

                item.ChangeName(command.Name);
                item.ChangeDescription(command.Description);

                unitOfWork.ItemRepository.Update(item);
                unitOfWork.Complete();
            }
        }

public PreconditionCheckResult PreconditionChecks(UpdateItemCommand command)
        {
            var preconditionCheckResults = new PreconditionCheckResult();

            if (command == null) throw new ArgumentNullException(nameof(command));

            using(var unitOfWork = _unitOfWorkFactory.CreateUnitOfWork())
            {
                var maybeItem = unitOfWork.ItemRepository.GetForId(command.Id);

                if (maybeItem.Any()==false)
                {
                    preconditionCheckResults.AddFailure(new PreconditionFailure(nameof(command.Id), string.Format("Cannot find Item for id: {0}", command.Id)));
                    return preconditionCheckResults;
                }
                var item = maybeItem.Single();

                var canChangeName = item.CanChangeName(command.Name);
                var canChangeDescription = item.CanChangeDescription(command.Description);

                if(canChangeName==false) preconditionCheckResults.AddFailure(nameof(command.Name), canChangeName.ErrorMessage);
                if (canChangeDescription == false) preconditionCheckResults.AddFailure(nameof(command.Description), canChangeDescription.ErrorMessage);
            }

            return preconditionCheckResults;
        }

UpdateItemModel class with data annotations

    public class UpdateItemModel
    {
        [Required]
        public Guid Id { get; set; } 
        [Required]
        public string Name { get; set; }
        public string Description { get; set; }
    }

Code has been pushed to the branch "develop-#34" (which is a rename of develop-#1)

[johncollinson2001]

Hi Joe,

I really like it, top stuff!

Just something to throw out there... I think one of the concerns mentioned in this thread was mapping command properties that have caused the failures to their corresponding model properties? At the mo the code above just returns the name of the command property in the model state.

If this is needed (and I think we should probably ensure model property names are returned where applicable) how about making the extension method model aware? E.g. preconditionsMet.Failures.AsModelState(). We could then use reflection to reference the model property names in the model state, based on matching name convention (e.g. command property name matches model property name), or with a custom attribute that's applied on the model properties when for example we want to map "Name" of the command to "PersonName" of the model, or perhaps "FullName" of the command needs a model state error raised for both "FirstName" and LastName" of the model.

Should no mapping be found (either by name convention or attribute), then just throw the error into the model state anyway with no property name, or some kind prefix/suffix/etc.

What do you think? I can knock up a prototype if needed.

[jadjare]

@johncollinson2001, I was thinking exactly the same thing with regard to the AsModelState method I just didn't have time to do it, so went for the crude implementation.

If you get time to do it by all means knock yourself out! Else I'll get to it sooner or later.. The code's up in the "develop-#34" branch... Please don't panic that all the classes for the implementation are in the wrong place, I was just prototyping!!

P.s. I will get some invites out soon for our next catch up!

[johncollinson2001]

Hi Joe,

Added the code in with a few tests to back it up.. moved the extension into it's own class so I could create it's own test case. Added a new attribute to be dotted on models for the mapping.

git pull :)