AF-3749: Loosen validation on Factory and meta initializers by corwinsheahan-wf · Pull Request #222 · Workiva/over_react

corwinsheahan-wf · 2019-01-09T00:42:57Z

Ultimate problem:

The new version of codemod will update the initializer for private factories to _$_Foo if the factory name is _Foo. We need to relax the validation on this initializer to allow for this.
It will do the same with the metaFor constant.

All of the following variations should be supported:

Factories:
- For a public factory:
  - $Foo
  - _$Foo
- For a private factory:
  - $_Foo
  - _$_Foo
  - _$Foo
Meta fields:
- For a public containing class:
  - $metaForFoo
  - _$metaForFoo
- For a private containing class:
  - $metaFor_Foo
  - _$metaFor_Foo
  - _$metaForFoo

Note: once the Dart 2 transition is complete, these restrictions will be tightened down to:

Factories:
- For a public factory:
  - _$Foo
- For a private factory:
  - _$_Foo
Meta fields:
- For a public containing class:
  - _$metaForFoo
- For a private containing class:
  - _$metaFor_Foo

How it was fixed:

Relax the validation + add tests
Moved validation for factory names to declaration parsing

Testing suggestions:

CI Passes
Publink into WSD and verify that running tests start without errors, since the validation will run before tests begin running.

Potential areas of regression:

False positive validation warnings on the already codemodded WSD. Hitting the testing suggestions will cover this possible regression.

FYA: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @evanweible-wf @sebastianmalysa-wf

aviary-wf · 2019-01-09T00:43:22Z

Security Insights

No security relevant content was detected by automated scans.

Action Items

Review PR for security impact; comment "security review required" if needed or unsure
Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

codecov-io · 2019-01-09T20:02:08Z

Codecov Report

Merging #222 into master will increase coverage by 0.2%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master    #222     +/-   ##
========================================
+ Coverage    90.1%   90.3%   +0.2%     
========================================
  Files          35      35             
  Lines        1767    1772      +5     
========================================
+ Hits         1592    1600      +8     
+ Misses        175     172      -3

sebastianmalysa-wf · 2019-01-09T20:11:03Z

lib/src/transformer/declaration_parsing.dart

 import 'package:transformer_utils/src/transformed_source_file.dart' show getSpan;
 import 'package:transformer_utils/transformer_utils.dart';

+


#nit extra line

sebastianmalysa-wf · 2019-01-09T20:18:01Z

test/vm_tests/transformer/impl_generation_test.dart

      });
    });

+


#nit extra line

sebastianmalysa-wf

Nice!! Looks good to me.

evanweible-wf · 2019-01-09T21:16:36Z

lib/src/transformer/declaration_parsing.dart

    }

+    // validate that the factory is initialized correctly
+    final factory = declarationMap[key_factory].length <= 1 ? singleOrNull(declarationMap[key_factory]) : null;


#nit should use a different variable name since factory is a language keyword

FYI factory just shows up weirdly formatted in GitHub, and isn't officially discouraged for use a variable name since it's a built-in identifier.

Oh, TIL! Thanks Greg :)

evanweible-wf · 2019-01-09T21:17:40Z

lib/src/transformer/declaration_parsing.dart

+    final factory = declarationMap[key_factory].length <= 1 ? singleOrNull(declarationMap[key_factory]) : null;
+    if (factory != null && factory is TopLevelVariableDeclaration) {
+      final String factoryName = factory.variables.variables.first.name.toString();
+      factory.variables.variables.forEach((variable) {


Should be sufficient to just look at the first variable in this declaration instead of iterating over the list

We should also probably validate that there's only one variable in the declaration, but that's unrelated to these changes PR. (this was cut-pasted)

That is already done in impl_generation, but I moved it to here, since we might as well fail earlier in that case.

evanweible-wf · 2019-01-09T21:18:44Z

lib/src/transformer/declaration_parsing.dart

+    // validate that the factory is initialized correctly
+    final factory = declarationMap[key_factory].length <= 1 ? singleOrNull(declarationMap[key_factory]) : null;
+    if (factory != null && factory is TopLevelVariableDeclaration) {
+      final String factoryName = factory.variables.variables.first.name.toString();


#nit my preference is to use a field if available rather than relying on a toString() method (it's not always clear whether or not toString() has been overridden to provide something meaningful), and in this case there should be a name field available:

Suggested change

final String factoryName = factory.variables.variables.first.name.toString();

final String factoryName = factory.variables.variables.first.name.name;

evanweible-wf · 2019-01-09T21:19:52Z

lib/src/transformer/declaration_parsing.dart

+          error(
+              'Factory variables are stubs for the generated factories, and should not have initializers '
+                  'unless initialized with a valid variable name for Dart 2 builder compatibility. '
+                  'Should be one of:\n    $expectedInitializers',


Instead of listing all of them (which sort of formalizes the support when really we want it to be temporary), let's provide the initializer value that we will eventually expect it to be

evanweible-wf · 2019-01-09T21:22:28Z

test/vm_tests/transformer/declaration_parsing_test.dart

+            test('_\$ prefixed variable name with private prefix stripped', () {
+              setUpAndParse('''
+                @Factory()
+                UiFactory<FooProps> _Foo = _\$_Foo;


private prefix isn't stripped here

Suggested change

UiFactory<FooProps> _Foo = _\$_Foo;

UiFactory<FooProps> _Foo = _\$Foo;

greglittlefield-wf · 2019-01-09T21:47:14Z

lib/src/transformer/declaration_parsing.dart

+        var expectedInitializers = ['\$metaFor${cd.name.name}', '_\$metaFor${cd.name.name}'];
+        if (isClassPrivate) {
+          expectedInitializers.add('_\$metaFor${cd.name.name.substring(1)}');
+        }


#nit would be nice to have aTODO or a comment saying which versions need to eventually be removed?

I think addressing #222 (comment) handles that effectively.

evanweible-wf · 2019-01-10T01:12:53Z

QA +1

CI passes
WSD consumer test passes: https://github.com/Workiva/web_skin_dart/pull/1120
- IE Interaction tests failed due to timeout, looks to be unrelated. Everything else passed.

@Workiva/release-management-p

allow for _ initializer on factories

0d99b32

corwinsheahan-wf requested a review from a team as a code owner January 9, 2019 00:42

rmconsole-wf added the Merge Requirements Unmet label Jan 9, 2019

Loosen checking for factories and metaFor

f108865

corwinsheahan-wf changed the title ~~AF-3749: Allow for _$_Foo initializer on factories~~ AF-3749: Loosen validation on Factory and meta initializers Jan 9, 2019

sebastianmalysa-wf reviewed Jan 9, 2019

View reviewed changes

test/vm_tests/transformer/impl_generation_test.dart Outdated

});

});

Copy link

Contributor

sebastianmalysa-wf Jan 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#nit extra line

sebastianmalysa-wf approved these changes Jan 9, 2019

View reviewed changes

evanweible-wf requested changes Jan 9, 2019

View reviewed changes

greglittlefield-wf reviewed Jan 9, 2019

View reviewed changes

address review feedback

4129dc3

greglittlefield-wf approved these changes Jan 9, 2019

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Jan 9, 2019

evanweible-wf approved these changes Jan 9, 2019

View reviewed changes

rmconsole-wf added the RM Ready label Jan 10, 2019

rmconsole3-wf merged commit e2756c5 into master Jan 10, 2019

rmconsole3-wf deleted the allow_for_private_factories branch January 10, 2019 01:13

rmconsole-readonly-wk mentioned this pull request Jan 10, 2019

AF-3738 AF-3737 Release over_react 1.30.2 #223

Merged

		import 'package:transformer_utils/src/transformed_source_file.dart' show getSpan;
		import 'package:transformer_utils/transformer_utils.dart';

	final String factoryName = factory.variables.variables.first.name.toString();
	final String factoryName = factory.variables.variables.first.name.name;

	UiFactory<FooProps> _Foo = _\$_Foo;
	UiFactory<FooProps> _Foo = _\$Foo;

Conversation

corwinsheahan-wf commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

Uh oh!

aviary-wf commented Jan 9, 2019

Security Insights

Action Items

Uh oh!

codecov-io commented Jan 9, 2019

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebastianmalysa-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

corwinsheahan-wf Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanweible-wf commented Jan 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

corwinsheahan-wf commented Jan 9, 2019 •

edited

Loading

corwinsheahan-wf Jan 9, 2019 •

edited

Loading