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

Add setting to blacklist types from nullable conversion #524

Closed
wants to merge 1 commit into from

Conversation

eb-delska
Copy link

Adds new global setting: Make Reference Types Nullable Blacklist
Closes #523 Customizable CustomTextWriter.InvalidStringsForPropertiesNeedingNullableTypes

After customizing code generation as part of migration from XrmToolkit it required ignoring more types from generated functions since conversion to nullable types happens after it.
Instead of hardcoding these, I added new setting to set a list of additional ignored types to avoid breaking existing code.
It could be extended to replace original list with default values and better handling of whitespace, but I left that as is for now.

@daryllabar
Copy link
Owner

Could I get a before and after for how this changes the generated code?

@daryllabar
Copy link
Owner

Also I'm struggling what the need for this particular feature is. You want nullable types, but not for all types, which I don't understand right now. @janis-veinbergs any details you'd like to share?

@eb-delska
Copy link
Author

eb-delska commented Nov 6, 2024

The simplest example why this would be needed are Guid types which are expected to be non nullable in our existing code, but it's probably only useful when using modified CustomizeCodeDomService.
Examples taken from Account entity:

  1. By default it generates System.Nullable<System.Guid> without any changes
[Microsoft.Xrm.Sdk.AttributeLogicalNameAttribute("processid")]
public System.Nullable<System.Guid> ProcessId
{
	get
	{
		return this.GetAttributeValue<System.Nullable<System.Guid>>("processid");
	}
	set
	{
		this.SetAttributeValue("processid", value);
	}
}
  1. We have a custom step that removes System.Nullable<> from it in extended CustomizeCodeDomService along with other changes resulting in the following before CustomTextWriter changes it:
[Microsoft.Xrm.Sdk.AttributeLogicalNameAttribute("processid")]
public System.Guid ProcessId
{
	get
	{
		return this.GetPropertyValue<System.Guid>("processid");
	}
	set
	{
		this.SetPropertyValue<System.Guid>("processid", value, nameof(ProcessId));
	}
}
  1. Without these changes and adding Guid to blacklist CustomTextWriter adds nullable back and the function ends up like this:
[Microsoft.Xrm.Sdk.AttributeLogicalNameAttribute("processid")]
public System.Guid? ProcessId
{
	get
	{
		return this.GetPropertyValue<System.Guid>("processid");
	}
	set
	{
		this.SetPropertyValue<System.Guid>("processid", value, nameof(ProcessId));
	}
}

We also generate additional methods to access OptionSetValue that's not expected to be nullable. Without blacklisting it also gets modified:

[AttributeLogicalName("industrycode")]
public OptionSetValue? IndustryCode_OptionSetValue
{
	get
	{
		return this.GetPropertyValue<OptionSetValue>("industrycode");
	}
	set
	{
		this.SetPropertyValue<OptionSetValue>("industrycode", value, nameof(IndustryCode_OptionSetValue));
	}
}

@janis-veinbergs
Copy link

@daryllabar it is to get away with less refactoring after xrmtoolkit migration

Xrmtoolit doesn't have nullable guids, OptionSetValues or EntityReferences. The default(T) is returned if attribute doesn't contain one rather than null.

However there are still properties that are nullable: bool?, DateTime? and some more.

@daryllabar
Copy link
Owner

daryllabar commented Nov 6, 2024

Ok, so you still want nullable properties for some types, so you want to keep the nullable types still being generated, but there are just a few property types where you don't. I see this as being a rather unusual edge case that very few other situations will call for. If we have 5 or more other configurations that will be required for the migrating from the XrmToollKit that no one else would need, I think we should create an XrmToollKit flag, or at least hide the configs unless really needed.

Any idea how many of these configuration changes will be required, that would be rather specific to the XrmToolKit? My guess is that it's really hard to know, so maybe we should start adding some, and then we will reassess if we want to change how it is configured later.

So with that being said, this should be the actual list of values, not a black list. I might be able to find time this week to update it.

@janis-veinbergs
Copy link

janis-veinbergs commented Nov 7, 2024

There are lot of customizations, but all of them can be achieved by our own ICustomizeCodeDomService by implementing things listed in #492. Sneak peek:

            new Entity.ChangeEntityBaseClass().CustomizeCodeDom(codeUnit, services);
            new Entity.RenameEntityFieldClass().CustomizeCodeDom(codeUnit, services);
            new Entity.RenameEntityProperties().CustomizeCodeDom(codeUnit, services);
            new Entity.ChangeEntityConstructor().CustomizeCodeDom(codeUnit, services);

            new Entity.SetTypesNonNullable(this).CustomizeCodeDom(codeUnit, services);
            new Entity.SetStringPropertyValueBounds(this).CustomizeCodeDom(codeUnit, services);
            new Entity.SetNumberPropertyValueBounds(this).CustomizeCodeDom(codeUnit, services);
            new Entity.SetGenericPropertyValue(this).CustomizeCodeDom(codeUnit, services);

            new Entity.ImproveOptionSet(this).CustomizeCodeDom(codeUnit, services);
            new Entity.PrimaryIdAttribute(this).CustomizeCodeDom(codeUnit, services);
            new Entity.ChangeMoneyMethods(this).CustomizeCodeDom(codeUnit, services);
            new Entity.AddSetState(this).CustomizeCodeDom(codeUnit, services);

It could all be buried under a single flag. However this is not 1:1 migration path from XrmToolkit, because:

  • XrmToolkit provides on itself options on how to configure generated code. So this is just for our configuration.
  • Simple code refactoring is still required, mainly because of different enum names and the way id properties are "synchronized".

But the code that we have is an enabler for migration.

Regarding this pullrequest - this is a change that impacts custom text writer service and we cannot influence for our side without recreating

There are 2 paths forward:

  • If this PR gets introduced and we will continue using our custom ICustomizeCodeDomService which inherits from yours to achieve our goals. We can also remove this configuration from UI not to overwhelm users as this is not very useful for greenfield projects.
  • If you propose that we introduce a single flag "GenerateXrmToolkitStyleCode", a PR can be prepared which includes ALL The changes necessary then we can do that. Probably this flag doesn't even need to be exposed in UI or it can, depending on how you view it.

@eb-delska eb-delska closed this Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customizable CustomTextWriter.InvalidStringsForPropertiesNeedingNullableTypes
3 participants