4

There are loads of resources for this on Google but I can't fully understand what I need to do in my scenario:

I have this class:

public class CompanyLanguage : EntityBase
{
    public int CompanyId { get; set; }
    public int LanguageId { get; set; }
    public bool IsDefault { get; set; }


    public virtual Company Company { get; set; }
    public virtual Language Language { get; set; }
}

Language is defined as:

public class Language:EntityBase
{
    [Required]
    [DisplayName("Language Code")]
    public string LanguageCode { get; set; }

    [Required]
    [MaxLength(2, ErrorMessage ="2 characters maximum")]
    [DisplayName("2 Char Language Code")]
    public string LanguageCode2Char { get; set; }

    [Required]
    [DisplayName("Language Name")]
    public string LanguageName { get; set; }

    public virtual List<LabelLanguage> LabelLanguages { get; set; }
}

Running a Fortify Scan returns the issue below as a high priority:

(ASP.NET MVC Bad Practices: Optional Submodel With Required Property)

We can't run the fortify scan - it's being run by someone else, so I need to get the changes right so it doesn't come straight back.

All the resources I've looked at suggest that underposting attacks could be made - i.e. a null Language, even though Language has some required properties.

For me, this is a valid scenario - the required properties of Language are only required if Language isn't null.

So what am I supposed to do to resolve this? Do I make public int LanguageId { get; set; } required, or public virtual Language Language { get; set; } or both?

Or am I completely wrong an I have to do something else? As I say I can't test these as the software has to be sent away for the test or I'd be trying all sorts out.

Percy
  • 2,855
  • 2
  • 33
  • 56
  • So you are sending this ef entity directly to a view without a view model? – Fran Nov 30 '16 at 15:24
  • I am - I've read that I should really be using view models but my view model would literally be exactly the same as the entity which kind of seems like I'm adding a lot more code and maintenance for not much reward. What do you think? – Percy Nov 30 '16 at 15:56
  • 2
    well the reward is separation of concerns and cleaner code. Since you are sending a many-to-many join entity to a view, you probably don't need all that information. it would be great to see the view and controller actions to see what you are doing, but I'm taking a guess that you've got a dropdowns for Company and Language. If that's the case the only thing to need to return to the view are lists of Id's and descriptions for each of the drop downs and not complete ef entities. – Fran Nov 30 '16 at 16:03
  • yes that's definitely true. I see where you're coming from. Do you think a lot of refactoring to incorporate view models is the best way to resolve my issue? Also, would that result in the Required attributes being removed from the Model and only used in the ViewModels? – Percy Nov 30 '16 at 16:06
  • 1
    yes, i believe that view models are the best way because they are modeling the information in the view. the view only requires that someone selects a langauge and/or company. they aren't creating a company which are the validation on the ef model. And yes there is a refactoring cost to this. Then you can marry up the view model and ef model in the controller action and even use linq queries to project directly into the view models. before they are sent to the view. – Fran Nov 30 '16 at 16:10
  • 1
    Here's a great explanation here http://stackoverflow.com/questions/14423056/why-do-we-use-viewmodels – Fran Nov 30 '16 at 16:11
  • To continue the discussion above, using a tool like automapper will greatly simplify the refactoring process to transfer data across your view model and your entity. – PmanAce Nov 30 '16 at 16:43
  • Automapper is nice, but i've moved away from it for my get viewmodels. I prefer to use linq queries to populate my view models directly. less configuration. i know automapper handles that now, but it does help in going from view model to domain model on POST. – Fran Nov 30 '16 at 16:46

1 Answers1

6

To summarize our discussion from comments.

  1. Create a view model which models only the information that is needed to satisfy the corresponding view.
  2. populate view models in your controller action from your domain ef models
  3. either project directly into view models using linq queries or Automapper.

Example view model for your question

public class CompanyLanguageEditViewModel
{
    [DisplayName("Company")]
    [Required]
    public int CompanyId { get; set; }

    [DisplayName("Language")]
    [Required]
    public int LanguageId { get; set; }
    public bool IsDefault { get; set; }

    public IEnumerable<SelectListItem> Companies{ get; set; }
    public IEnumerable<SelectListItem> Languages { get; set; }
}

And in your view you can then use 

@Html.DropDownListFor(x => x.CompanyId, Model.Companies);

and your label will be Country and you are only going to POST back what you need

Fran
  • 6,440
  • 1
  • 23
  • 35
  • Definitely the way I should have gone! As a quick fix, to get through this issue, would making `public virtual Language Language { get; set; }` a required property work? Is that valid? As I say, I can't try it and see as I'm not performing the test. Time is short and making all these ViewModel changes is going to mean not meeting the deadline. – Percy Dec 01 '16 at 09:43
  • How does that get you around your original problem? Your still passing an ef model to your view. Also the virtual is unnecessary. That virtual is so ef can return a proxy object for lazy loading. And I'm fairly certain if you mark Language as required, all the properties of Language that are required are going to be required when you post – Fran Dec 01 '16 at 12:24
  • The original problem is that the submodel (Language) has required properties even though Language is optional in CompanyLanguage. So I would assume making Language required, would solve the issue? – Percy Dec 01 '16 at 15:11
  • so if those Language properties are required are you putting all the language properties in hidden fields and posting them back too? wouldn't the model binder still throw validation errors because you aren't actually sending back the LanguageCode or LanguageCode2Char back? – Fran Dec 01 '16 at 15:22
  • Thanks, I'm half way through the refactoring - it's a much better solution. – Percy Dec 08 '16 at 14:54