[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

clrudolphi · 2024-09-09T19:28:03Z

🤔 What's changed?

Modified the Dotnet template.
The template was generating code that was intolerant of arguments that were null when it expected a List.
Added a test case for the regression.

🏷️ What kind of change is this?

🐛 Bug fix (non-breaking change which fixes a defect)

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- [x ] I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- [x ] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

…tructor arguments The template was generating code that was intolerant of arguments that were null when it expected a List<T>. Added a test case for the regression.

mpkorstanje · 2024-09-09T19:39:13Z

dotnet/Cucumber.Messages/generated/Background.cs

@@ -45,7 +45,7 @@ string id
              RequireNonNull<string>(description, "Description", "Background.Description cannot be null");
        this.Description = description;
              RequireNonNull<List<Step>>(steps, "Steps", "Background.Steps cannot be null");
-        this.Steps = new List<Step>(steps);
+        this.Steps = steps == null ? new List<Step>() : new List<Step>(steps);


How can steps be null? It is guarded by RequireNonNull on the line above.

Hmm. Hadn't been dealing with Background yet, so hadn't seen that. At worst, it's a bit redundant.

But look at TestStep (before my proposed change):

public TestStep( string hookId, string id, string pickleStepId, List<string> stepDefinitionIds, List<StepMatchArgumentsList> stepMatchArgumentsLists ) { this.HookId = hookId; RequireNonNull<string>(id, "Id", "TestStep.Id cannot be null"); this.Id = id; this.PickleStepId = pickleStepId; this.StepDefinitionIds = new List<string>(stepDefinitionIds); this.StepMatchArgumentsLists = new List<StepMatchArgumentsList>(stepMatchArgumentsLists); }

The schema here indicates that the list of StepDefinitionIds and the list of StepMatchArgumentsLists are optional.
If this code gets invoked with null for either, the code throws an exception as the List constructor, when given an argument, the argument must not be null.

I added a test in BasicMessageSerializationTests.cs to demonstrate the behavior.

Would you prefer that I make the template a bit more sophisticated to use the new pattern only when the property is optional?

Ah. Yes. More sophisticated would be preferred. You can probably copy this from Java.

Modified the codegen so that constructor argument arrays do not use the conditional check for null (as the NotNull check on the previous line would have done that already).

clrudolphi · 2024-09-09T20:45:21Z

Modified the generation so that for arguments that are required, the check for null is not done when invoking the new List.

clrudolphi · 2024-09-09T20:47:07Z

Can I get some assistance on how to control indenting of the generated lines of code? It's not intuitive to me why some lines are generated at different indent levels. It must have something to do with being embedded in <%- if -%> conditions, but I'm not proficient on this templating language.

mpkorstanje · 2024-09-09T21:18:18Z

You can find an overview of the ERB syntax here. https://www.puppet.com/docs/puppet/5.5/lang_template_erb

It is not the canonical documentation, but much better explained.

But it looks like <% if required -%> should trim to the preceding new line. So <%- if required -%> should be used. Then the lines that are rendered can be put on the correct indentation level.

mpkorstanje · 2024-09-09T21:23:56Z

dotnet/Cucumber.Messages/generated/TestStep.cs

        this.Id = id;
              this.PickleStepId = pickleStepId;
-              this.StepDefinitionIds = new List<string>(stepDefinitionIds);
-              this.StepMatchArgumentsLists = new List<StepMatchArgumentsList>(stepMatchArgumentsLists);
+              this.StepDefinitionIds = stepDefinitionIds == null ? new List<string>() : new List<string>(stepDefinitionIds);


I think this should be:

Suggested change

this.StepDefinitionIds = stepDefinitionIds == null ? new List<string>() : new List<string>(stepDefinitionIds);

this.StepDefinitionIds = stepDefinitionIds == null ? null : new List<string>(stepDefinitionIds);

Similar to https://github.com/cucumber/messages/blob/de3f88f1b7fa43ebceeb6e247767e44ff8c18730/java/src/generated/java/io/cucumber/messages/types/TestStep.java#L36C1-L36C122

The field is not required, so it can be null. When serializing, if configured correctly, this will result in the omission of the field. And so it would be strange if non-required value that wasn't present suddenly became an empty list after serialisation.

In principle, I agree with you, purely optional elements should be null and not emitted when serialized.
I'm curious then about these lines in the Hooks example in the CCK:

{"testCase":{"id":"33","pickleId":"20","testSteps":[{"hookId":"0","id":"29"},{"hookId":"1","id":"30"},{"id":"31","pickleStepId":"19","stepDefinitionIds":["2"],"stepMatchArgumentsLists":[{"stepMatchArguments":[]}]},{"hookId":"4","id":"32"}]}} {"testCase":{"id":"38","pickleId":"22","testSteps":[{"hookId":"0","id":"34"},{"hookId":"1","id":"35"},{"id":"36","pickleStepId":"21","stepDefinitionIds":["3"],"stepMatchArgumentsLists":[{"stepMatchArguments":[]}]},{"hookId":"4","id":"37"}]}} {"testCase":{"id":"43","pickleId":"24","testSteps":[{"hookId":"0","id":"39"},{"hookId":"1","id":"40"},{"id":"41","pickleStepId":"23","stepDefinitionIds":[],"stepMatchArgumentsLists":[]},{"hookId":"4","id":"42"}]}}

In the first two lines, the third step of each test case is matched with a StepDefinition; each of those stepDefinitions accept no parameters. So why are the stepMatchArgumentsLists and stepMatchArguments properties being serialized as empty lists? Why not omit them altogether? (They are optional according to the schema).
Likewise in the third example (the test case does not bind to any step definition (aka, pending). Why is the StepDefinitionIds array getting serialized when it is empty?

I will try out your proposal and see how well it works with Reqnroll.

A TestStep is either a PickleTestStep or a HookTestStep.

When a PickleTestStep then the id, pickleStepId, stepDefinitionIds and stepMatchArgumentsLists are required.

When a HookTestStep then the id and hookId are required.

So in the union of these, only the id is required.

There are now better ways to express this in a json schemas but either we didn't have those at the time, or they didn't quite work.

Oof. That makes a general solution almost impossible.
If the template were to deserialize missing optional lists as null then they won't round-trip correctly. (Nor would it round-trip correctly with the code as it exists or with what I had proposed)
I'll consider inserting a patch up phase in the Serializer helper class.

How does it make round tripping correctly impossible? Not including null values when serializing and not replacing absent/null values with empty list during deserialisation is a successful round trip.

Or do you mean with the current implementation?

Well, OK, not impossible.
Under good conditions, the round-trip would be successful.
Under the principle of being forgiving of bad input and strict on output; if the input json string (for a Pickle TestStep) were to omit the stepDefinitionIds instead of representing it as an empty array, then this code would set the StepDefinitionIds property to null, and render it back out during serialization as null (rather than the required empty array).
Are those edge cases something that we should be guarding against?

There might be a use-case for it, but it would be somewhat out of scope for this PR.

In this example, an error correction to an empty list would be context sensitive. The correct solution would depend on whether we are dealing with a PickleTestStep or a HookTestStep.

Given that sort of complexity, probably best discussed separately, if at all.

mpkorstanje · 2024-09-09T21:27:01Z

Oh btw. You should have the rights to make branches directly in this repository. You don't need to fork, that should avoid some of the work that comes with keeping the fork up to date.

mpkorstanje · 2024-09-09T21:28:51Z

codegen/templates/dotnet.dotnet.erb

@@ -73,10 +73,14 @@ public sealed class <%= class_name(key) %>
          required = (schema['required'] || []).index(property_name) 
      -%>
      <% if required -%>


Suggested change

<% if required -%>

<%- if required -%>

mpkorstanje · 2024-09-09T21:30:05Z

codegen/templates/dotnet.dotnet.erb

@@ -73,10 +73,14 @@ public sealed class <%= class_name(key) %>
          required = (schema['required'] || []).index(property_name) 
      -%>
      <% if required -%>
-        RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");
+  RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");


Suggested change

RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");

RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");

mpkorstanje · 2024-09-09T21:36:30Z

I think I marked the likely culprit of the indentation problems. Can't test this. I haven't got my regular development machine available right now.

clrudolphi · 2024-09-09T22:18:24Z

Thanks for the pointers and suggestions. I'll work on them tomorrow.

luke-hill · 2024-09-10T10:16:23Z

@clrudolphi if you need any help with erb templating we can pair on it some time unless you've cracked it here. I can also explain the diff semantics on tags better if needed?

@mpkorstanje

Cleaned up formatting produced by the dotnet template. Adopted @mpkorstanje suggestion on how to handle null values within constructors for optional properties that are Lists.

clrudolphi · 2024-09-12T02:10:12Z

@mpkorstanje accepted your suggestion of how to handle the nulls and fixed up the indent formatting.

…_InGeneratedCode

mpkorstanje · 2024-09-12T06:56:35Z

CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]
+- [Dotnet] Fixed code generation for types that accept List<T> as parameters. Constructors were not properly handling null input. [clrudolphi]


Spotted this after merging. Changes should be listed as one of Added, Changed, ect. I'll fix it on main.

[DotNet] Fix generation of classes that accept optional lists as cons…

bf1407e

…tructor arguments The template was generating code that was intolerant of arguments that were null when it expected a List<T>. Added a test case for the regression.

mpkorstanje reviewed Sep 9, 2024

View reviewed changes

Modified for Required Lists

fc7edd5

Modified the codegen so that constructor argument arrays do not use the conditional check for null (as the NotNull check on the previous line would have done that already).

clrudolphi requested a review from mpkorstanje September 9, 2024 20:47

mpkorstanje reviewed Sep 9, 2024

View reviewed changes

mpkorstanje mentioned this pull request Sep 11, 2024

Document that a TestStep is either a PickleTestStep or a HookTestStep #250

Open

Fix gen of classes that accept optional lists as constructor args

b099347

Cleaned up formatting produced by the dotnet template. Adopted @mpkorstanje suggestion on how to handle null values within constructors for optional properties that are Lists.

mpkorstanje self-requested a review September 12, 2024 06:18

Merge branch 'main' into DotNet_FixHandlingOfNullConstructorArguments…

fc7ee47

…_InGeneratedCode

mpkorstanje merged commit 705d19f into cucumber:main Sep 12, 2024
26 checks passed

mpkorstanje reviewed Sep 12, 2024

View reviewed changes

clrudolphi deleted the DotNet_FixHandlingOfNullConstructorArguments_InGeneratedCode branch September 12, 2024 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

clrudolphi commented Sep 9, 2024

mpkorstanje Sep 9, 2024

clrudolphi Sep 9, 2024

mpkorstanje Sep 9, 2024

clrudolphi commented Sep 9, 2024

clrudolphi commented Sep 9, 2024

mpkorstanje commented Sep 9, 2024 •

edited

Loading

mpkorstanje Sep 9, 2024 •

edited

Loading

clrudolphi Sep 10, 2024

mpkorstanje Sep 11, 2024

clrudolphi Sep 11, 2024

mpkorstanje Sep 11, 2024

clrudolphi Sep 11, 2024 •

edited

Loading

mpkorstanje Sep 11, 2024 •

edited

Loading

mpkorstanje commented Sep 9, 2024

mpkorstanje Sep 9, 2024

mpkorstanje Sep 9, 2024

mpkorstanje commented Sep 9, 2024

clrudolphi commented Sep 9, 2024

luke-hill commented Sep 10, 2024

clrudolphi commented Sep 12, 2024

mpkorstanje Sep 12, 2024 •

edited

Loading

	this.StepDefinitionIds = stepDefinitionIds == null ? new List<string>() : new List<string>(stepDefinitionIds);
	this.StepDefinitionIds = stepDefinitionIds == null ? null : new List<string>(stepDefinitionIds);

	RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");
	RequireNonNull<<%= type_for(class_name(key), property_name, property) -%>>(<%= property_name %>, "<%= capitalize(property_name) %>", "<%= class_name(key) %>.<%= capitalize(property_name) %> cannot be null");

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

[DotNet] Fix generation of classes that accept optional lists as constructor arguments #249

Conversation

clrudolphi commented Sep 9, 2024

🤔 What's changed?

🏷️ What kind of change is this?

📋 Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clrudolphi commented Sep 9, 2024

clrudolphi commented Sep 9, 2024

mpkorstanje commented Sep 9, 2024 • edited Loading

mpkorstanje Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clrudolphi Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

mpkorstanje Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

mpkorstanje commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje commented Sep 9, 2024

clrudolphi commented Sep 9, 2024

luke-hill commented Sep 10, 2024

clrudolphi commented Sep 12, 2024

mpkorstanje Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

mpkorstanje commented Sep 9, 2024 •

edited

Loading

mpkorstanje Sep 9, 2024 •

edited

Loading

clrudolphi Sep 11, 2024 •

edited

Loading

mpkorstanje Sep 11, 2024 •

edited

Loading

mpkorstanje Sep 12, 2024 •

edited

Loading