Fix topological sort NPE when re-enqueueing shapes

[Jacoby6000]

Background

I have, on several occasions, run in to issues with an NPE coming out of the TopologicalShapeSort class. Specifically:

Caused by: java.lang.NullPointerException: Cannot invoke "java.util.Set.remove(Object)" because "dependentDependencies" is null
    at software.amazon.smithy.model.loader.TopologicalShapeSort.dequeueSortedShapes(TopologicalShapeSort.java:83)
    at software.amazon.smithy.model.transform.ReplaceShapes.updateMixins(ReplaceShapes.java:183)
    at software.amazon.smithy.model.transform.ReplaceShapes.transform(ReplaceShapes.java:80)
    at software.amazon.smithy.model.transform.ModelTransformer.replaceShapes(ModelTransformer.java:102)
    ...

In some of these situations, I was able to alleviate the problem by changing the order in which I perform my model transformations, but I finally ran in to a case where I could not. I decided to look a little deeper.

When running dequeueSortedShapes, each shape that has no dependencies is processed in the order that they are enqueued. Upon dequeuing, they are removed from the forward dependencies map. This is fine when satisfiedShapes only contains satisfied shapes, but presents issues when something with an unsatisfied dependency is marked as satisfied. In this scenario, a given dependent may be processed before its dependencies and results in the case below:

• B is enqueued with no dependencies
• A is enqueued with no dependencies
• B is then re-enqueued, with a dependency on A
• ...
• dequeueSortedShapes is called
• B is processed as a satisfied shape immediately
  • Removes itself from forward dependencies
  • Has no reverse dependencies, so this shape is done processing
• A is processed as a satisfied shape
  • Removes itself from forward dependencies
  • Detects B as a reverse dependency
  • Attempts to fetch the forward dependencies of B
  • forward dependencies are null, because B was already processed

The solution winds up being very simple. When a shape is enqueued with dependencies, we should check if it was previously marked as satisfied, and remove it from the queue if it was.

A related/opposite case was identified here: #2785 (comment)
This PR fixes that case as well.

Testing

• I reproduced the failure using model interactions exclusively, then reproduced the failure directly with the topological sorter.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[Jacoby6000]

Another potential solution may be to change the way that ReplaceShapes works, such that a shape with a duplicate Id is never enqueued in to the topological sorter more than once. Though the sorter should probably document somewhere that it expects each shape to be enqueued exactly once and throw, if this is an expected restriction. Or maybe return a boolean

[sugmanue]

Hi Jacob, thank you for the fix, we will get someone assigned to review it, but can you please fix the build failures, should be as easy as running ./gradlew :smithy-model:spotlessApply and committing the changes.

[Jacoby6000]

Hi Jacob, thank you for the fix, we will get someone assigned to review it, but can you please fix the build failures, should be as easy as running ./gradlew :smithy-model:spotlessApply and committing the changes.

Done!

[JordonPhillips]

I have a pr up that created a centralized dependency graph implementation - perhaps it should be used here. But in the meantime this looks good.

[Jacoby6000]

I have a pr up that created a centralized dependency graph implementation - perhaps it should be used here. But in the meantime this looks good.

Agreed about switching to your implementation when it is ready.. There's only one or two classes that use this topological sort class, so it shouldn't be too hard to adapt them to use your implementation.