CPLAT-9308 Codemod Utility to tell if Something is Public API

[sydneyjodon-wk]

Motivation

To support CPLAT-9205, we need a way to tell whether something is a public API.

• Don't worry about generating the semver report in this ticket. We can either run semver_audit or download the appropriate report in the repo_commander script. And then the codemod can assume that it exists at a "report.json" file or something

• Use a local semver_audit report to tell whether a component class is part of the public API. What the report looks like:

  {
      "exports": {
         "lib/web_skin_dart.dart/ButtonProps": {
            "type": "class",
            "grammar": {
                "name": "ButtonProps",
                "meta": ["@Props()"]
            }
         }
      }
  }

• Add a function (likely encapsulated in a class, or whatever makes sense):

  List<String> getPublicExportLocations(CompilationUnitDeclaration node) { ... }

  • Return a List instead of a bool so that so it can output helpful messages saying why a component wasn't codemodded, and also in case the codemod wants to perform conditional logic based on the name of exports.

• Make it possible to call this function repeatedly re-parsing the semver report (which can be upwards of 10MB for some repos)

  • E.g.,

    var helper = SemverHelper.fromReport(reportJson);
    ...
    var suggestors = [
      Suggestor1(semverHelper: helper),
      Suggestor2(semverHelper: helper),
    ];

Changes

• Write SemverHelper class with getPublicExportLocations utility.
• Write tests.
• Create simple report.json mock to help with testing codemod suggestors in other tickets.
• Add tests to existing migrators.

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review: @aaronlademann-wf @joebingham-wk @greglittlefield-wf

QA Checklist

• Tests were updated and provide good coverage of the changeset and other affected code
• Manual testing was performed if needed
  • Steps from PR author:
  • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

• A Client Platform member has reviewed these changes
• There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
• For release PRs - Version metadata in Rosie comment is correct

[aviary3-wk]

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.

[greglittlefield-wf]

I wrote some comments without realizing that this wasn't ready for review yet 🤦‍♂ Sorry about that!

I figured I'd post the comments anyways, but take them with a grain of salt!

[greglittlefield-wf]

We need a version of the report tracked in Git somehow, as opposed to this hard-coded API.

To make testing easier, perhaps having a constructor that takes in the map directly would be helpful. That way, you can embed the JSON in the test, and also choose how to load the report in actual usage.

SemverHelper.fromReport(this.exportList);

// in the test for SemverHelper
final helper = SemverHelper({'exports': ...});

// in usage:
final helper = SemverHelper(jsonDecode(await File('report.json').readAsString()));

[greglittlefield-wf]

Also, a test for a Suggestor would be a case where we'd want to use the mock for SemverHelper as opposed to a real instance, to make test setup easier (by not having to set up the whole exports map).

[joebingham-wk]

Just a couple early thoughts! :)

[joebingham-wk]

This suggestion builds on code that isn't here yet (it would come from Aaron's branches), but how would you feel about integrating the helper into an isPublic function? I think the implementation could be something like:

boilerplate_utilities.dart

// Create a helper that the utilities can see
SemverHelper semverHelper;

bool isPublic(ClassDeclaration node) {
  assert(semverHelper != null);
  return semverHelper.getPublicExportLocations(node) != null;
}

bool shouldMigratePropsAndStateClass(
    ClassDeclaration node) {
  return isAssociatedWithComponent2(node) &&
      isAPropsOrStateClass(node) && !isPublic(node);

boilerplate_upgrade.dart

semverHelper = SemverHelper(jsonDecode(
      await File('lib/src/boilerplate_suggestors/report.json').readAsString()));

The main thing I'm trying to prevent is passing around the same helper instance through all the suggestors and utilities, but if that seems like a good goal then I'm open to whatever implementation works best!

[greglittlefield-wf]

The main thing I'm trying to prevent is passing around the same helper instance through all the suggestors and utilities, but if that seems like a good goal then I'm open to whatever implementation works best!

Passing around the instance is actually what I'd prefer, as opposed to a global variable.

Advantages:

• Eliminates the risk of different areas of the code unintentionally interfering with each other's data (including in tests)
  -Make it easier to reason about where this is used, and force us to be intentional about where we pass it

[joebingham-wk]

I could definitely see that, and normally I wouldn't advocate for a global variable, but my main thought is that the function which answers the question of whether a node is public or not is at the utility level, and it doesn't seem like the suggestor should need to be concerned with how it answers that question (outside of passing in the node).

Also, because this helper should receive the report for the relevant library it's running on, it seems like it makes sense for it to operate independently since any instantiation for an executable should be passing it the same report. For tests that does break down, but then the testing environment could have its own instance when needed. If you feel strongly, Greg, I think that generally global variables are bad, this is just a weird case to me 😄

[greglittlefield-wf]

I feel pretty strongly here that moving away from a top-level variable would improve the modularity of this code, as well as the predictability of how data is passed around and used.

it seems like it makes sense for it to operate independently since any instantiation for an executable should be passing it the same report

That's true, but may not always be true. For example, what if we wanted this utility to behave slightly differently for different suggestors? Or if we wanted to run multiple sets of suggestors on different directories concurrently? Having global variables would make it much more difficult to implement these changes. To be fair, this is the weakest argument of the ones I've presented, but it is something to keep in mind.

---

I'm happy to make a PR with those changes, especially since some work was already done to move it more toward top-level variables.

[joebingham-wk]

Moving away from a global variable sounds good to me. Thanks for explaining your logic, Greg! And sorry for making ya change it in the first place Sydney 😅

[joebingham-wk]

It seems like we may want this to be a louder failure. Have we seen the class name be null before? I'd be curious to see what that code looks like haha

If these can be null though, I'd expect it to throw an Exception since we need the list and class name to really verify if the class is exported, and returning null could be a false negative

[greglittlefield-wf]

+10, assuming that #74 (comment) is addressed in your other PR #80

stale