modelsbuilder / ModelsBuilder.Original

The Community Models Builder for Umbraco
MIT License
114 stars 49 forks source link

Generating models using the VS extension can cause namespace issues #228

Open tompipe opened 5 years ago

tompipe commented 5 years ago

When using the VS extension and the .mb file to generate models, the extension generates namespaces using the .mb file's RootNamespace and a relative namespace generated from the filesystem folders. https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/12aec7920ea45da5af0d1271197620cbdcb48267/src/ZpqrtBnk.ModelsBuilder.Extension/Generator.cs#L70-L78

This generated namespace is sent through to the API to build models. Although the namespace can be overridden with either the ModelsNamespaceAttribute or the Umbraco.ModelsBuilder.ModelsNamespace appsettings value, it appears there is a case where this isn't being used, and is falling back to the namespace passed to the API by the extension.

I think the issue is caused by the Builder class which passes through the namespace passed in from the VS extension through to the MapModelTypes method. https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/bf0da79aee8f52ec153bbb200131d1914ee95b2b/src/Umbraco.ModelsBuilder/Building/Builder.cs#L115

https://github.com/zpqrtbnk/Zbu.ModelsBuilder/blob/bf0da79aee8f52ec153bbb200131d1914ee95b2b/src/Umbraco.ModelsBuilder/Building/TypeModel.cs#L212-L223

This results in models being generated with the namespace passed in from the VS extension, which could become invalid if the attribute, or appsettings value modifies the namespace, e.g:

image

[assembly: ModelsNamespace("Website.Core.Models")]
namespace Website.Core.Models
{
  /// <summary>Homepage</summary>
  [PublishedModel("homepage")]
  public partial class Homepage : PublishedContentModel
  {
    // snip

    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Umbraco.ModelsBuilder", "8.1.0")]
    [ImplementPropertyType("footerSocialMediaLinks")]
    public IEnumerable<Website.Core.Models.Generated.SocialMediaLinksSchema> FooterSocialMediaLinks => this.Value<IEnumerable<Website.Core.Models.Generated.SocialMediaLinksSchema>>("footerSocialMediaLinks");
  }
}

When this is what was expected

[assembly: ModelsNamespace("Website.Core.Models")]
namespace Website.Core.Models
{
  /// <summary>Homepage</summary>
  [PublishedModel("homepage")]
  public partial class Homepage : PublishedContentModel
  {
    // snip

    [global::System.CodeDom.Compiler.GeneratedCodeAttribute("Umbraco.ModelsBuilder", "8.1.0")]
    [ImplementPropertyType("footerSocialMediaLinks")]
    public IEnumerable<SocialMediaLinksSchema> FooterSocialMediaLinks => this.Value<IEnumerable<SocialMediaLinksSchema>>("footerSocialMediaLinks");
  }
}
zpqrtbnk commented 5 years ago

So... the extension always send a namespace, which derives from the filesystem structure. And then in the controller we do:

var generator = new Generator(_umbracoServices, _codeFactory, _config);
var models = generator.GetModels(data.Namespace, data.Files);

I.e. we pass that data.Namespace to the generator... and since it is not null, it is used, instead of the one in _config. This is because "in the old time" the extension may send a null namespace (and then we would use the config), or a non-null namespace (meaning, we really want to use that one). But then the extension changed to always send a namespace. And so... it cannot come from the config anymore.

This is annoying - I think the fix would be to reverse the order. Take the namespace from config, if any, else use what the extension sends.

Thoughts?

hfloyd commented 4 years ago

This is annoying - I think the fix would be to reverse the order. Take the namespace from config, if any, else use what the extension sends.

Thoughts?

I think that this is a good solution. If an explicit namespace is provided via the config, then we want to use that. If there is no namespace in the config, then we can use the "default" (based on the folder structure). This would ensure that if a dev didn't add any special config it would "just work", but if a dev sets the config, that config would be respected across the code generation.

hfloyd commented 3 years ago

I am bumping up against this issue continually and would love it to be fixed. Is there some way I can help?