Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tompipe opened this issue Nov 12, 2019 · 3 comments
Open

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

tompipe opened this issue Nov 12, 2019 · 3 comments
Labels
state/in-progress Work In Progress type/bug Bug
Milestone

Comments

@tompipe
Copy link

tompipe commented Nov 12, 2019

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
Copy link
Collaborator

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
Copy link

hfloyd commented Jul 21, 2020

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
Copy link

hfloyd commented Mar 18, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/in-progress Work In Progress type/bug Bug
Projects
None yet
Development

No branches or pull requests

3 participants