2
\$\begingroup\$

I have a controller for Post and Put request for the same entity: MyModelDTO with some ModelState's attributes.

The users can make a Post request without the Id attribute, because it is being generated on server side, but for Put request the Id attribute is needed (And has to be different than an empty guid).

I want to allow get a Post request without the "Id" property, and still use the ModelState functionality, the best solution I've came across is overriding the OnActionExecuting and removing the specific key:

public class ExceptPropertiesAttribute : ActionFilterAttribute
{
    private IEnumerable<string> _propertiesKeys;

    public ExceptPropertiesAttribute(string commaSeperatedPropertiesKeys)
    {
        if (!string.IsNullOrEmpty(commaSeperatedPropertiesKeys))
        {
            this._propertiesKeys = commaSeperatedPropertiesKeys.Split(',');
        }
    }

    public override void OnActionExecuting(ActionExecutingContext actionContext)
    {
        if (this._propertiesKeys != null)
        {
            foreach (var propertyKey in this._propertiesKeys)
            {
                if (actionContext.ModelState.ContainsKey(propertyKey))
                {
                    actionContext.ModelState.Remove(propertyKey);
                }
            }                
        }
    }
}

MyModelDTO class:

public class EventModelDTO : IBaseModel
{
    [IsNotEmpty(ErrorMessage = "Guid Id Is Empty")]
    public Guid Id { get; set; }
    [Required]
    public string Name { get; set; }
}

public class IsNotEmptyAttribute : ValidationAttribute
{
    public override bool IsValid(object value)
    {
        if (value == null) return false;

        var valueType = value.GetType();
        var emptyField = valueType.GetField("Empty");

        if (emptyField == null) return true;

        var emptyValue = emptyField.GetValue(null);

        return !value.Equals(emptyValue);
    }
}

Usage:

[HttpPost]
[ExceptPropertiesAttribute("Id")]
public ActionResult Post([FromBody] MyModelDTO itemDTO = null)
{
    try
    {
        if (!ModelState.IsValid)
        {
            return BadRequest();
        }

        MyModel item = _mapper.Map<MyModel>(itemDTO);
        ResultService result = this._myService.Insert(item);
        if (result.Success)
        {                    
            return CreatedAtAction("Post", new { item.Id }, itemDTO);
        }
        else
        {
            return StatusCode(500, result.ErrorCode);
        }
    }
    catch (Exception ex)
    {
        return StatusCode(500, ErrorCode.Unknown);
    }
}

[HttpPut("{id}")]
public ActionResult Put([FromBody] MyModelDTO itemDTO)
{
    try
    {                
        if (!ModelState.IsValid)
        {
            return BadRequest();
        }

        MyModel item = _mapper.Map<MyModel>(itemDTO);
        ResultService result = this._eventsService.Update(item);
        if (result.Success)
        {
            return Ok();
        }
        else
        {
            return StatusCode(500, result.ErrorCode);
        }
    }
    catch (Exception ex)
    {
        return StatusCode(500, ErrorCode.Unknown);
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ How is ModelState bound to an Id that you need this workaround? Please describe the problem you are solving with more details. \$\endgroup\$ – t3chb0t Jun 17 at 15:29
  • \$\begingroup\$ @t3chb0t see updated question, it checks if the Id is not null or empty guid \$\endgroup\$ – Shahar Shokrani Jun 17 at 16:09
  • 1
    \$\begingroup\$ If I understand it correctly your entities have an Id property that is decorated with the IsNotEmptyAttribute and for certain requests you'd like to exclude this validation of the model? \$\endgroup\$ – t3chb0t Jun 17 at 16:12
  • \$\begingroup\$ Hey guys, see updated question \$\endgroup\$ – Shahar Shokrani Jun 17 at 17:52
2
\$\begingroup\$

While your approach might work, I consider it a hack. The preferred solution would be to create different models for each request so that the user does not have a chance of passing values that might not only not be required but even be forbidden.

Your new entities could be:

class NewUser
{
    public string Name { get; set; }
}

Where there are is no Id if the AIP does not require it.


It is a very bad idea to expose your database models to the caller. Create more specific models and map their properties to your database entities.

If you let the user the possibility of passing anything he wants, you would need to validate even more values to be sure he does not try to do something else than you want him to.

One such example could be when you have an API for updating User.Name but that user has a navigation property Books and the caller sends you some books too that will also get updated... just because he can. This might cause you a lot of trouble.

\$\endgroup\$
  • \$\begingroup\$ So you are saying to create a MyModelPostRequest and a MyModelPutRequest, I didn't exposed my db model to users, see the DTO suffix and the Mapper in updated question. \$\endgroup\$ – Shahar Shokrani Jun 17 at 17:55
  • \$\begingroup\$ @ShaharShokrani you might not use db-entities directly but these dtos look a lot like the entities; I bet they are identical... if they weren't then you wouldn't have to create workarounds for ignoring some properties that obviously don't belong there in certain requests. \$\endgroup\$ – t3chb0t Jun 17 at 17:59
  • 1
    \$\begingroup\$ I see, agreed, the lesson for me is to create separate request classes for Put and Post, leave the DTO only for Get request... \$\endgroup\$ – Shahar Shokrani Jun 17 at 18:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.