Fix: broken namespace aliases and mutable selections

[gmac]

Fixes some problems with namespace operations in the query planner, and simplifies the overall logic in the process.

Problems + Fixes

query {
  boo: myNamespace {
    product {
      name
    }
    manufacturer {
      name
    }
  }
}

• The namespace handler condition was demonstrably broken by field aliases: it composed its insertion point using field Name rather than Alias. This issue is now fixed.

• The namespace condition could also corrupt the user's original selection because it modified fields directly rather than making copies. This was problematic because it modified the selection used to filter the final result, as described in https://github.com/movio/bramble/issues/106. This is also fixed.

• With the above adjustments made, the namespace condition ended up basically identical to the normal composite field handler. So, this simplifies the logic so that a failed location lookup now just flows into the normal field handlers; this new pattern has the added advantage of handing both leaf values and composite fields.

Resolves https://github.com/movio/bramble/issues/106 by happy accident.

[codecov-commenter]

Codecov Report

Merging #108 (81cb6ac) into main (e0e24cc) will increase coverage by 0.81%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   67.67%   68.48%   +0.81%     
==========================================
  Files          24       24              
  Lines        2738     2618     -120     
==========================================
- Hits         1853     1793      -60     
+ Misses        734      690      -44     
+ Partials      151      135      -16     

Impacted Files	Coverage Δ
plan.go	83.78% <100.00%> (+0.30%)	:arrow_up:
format.go	86.84% <0.00%> (+19.14%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e0e24cc...81cb6ac. Read the comment docs.

[lucianjon]

Thanks @gmac, looks like an improvement on what we had. I'd like to refactor that whole piece at some point, as assuming the error is actually not an error makes the conditions read quite weird. That would require another way to figure out the field requested is a namespace, which I'm sure is possible but just requires some thinking.

For now would you mind chucking a comment in the else case to say the error is ignored as it's assumed to be a namespace if err != nil. Just to make sure someone else coming along doesn't think the error is unintentionally ignored. We're still ignoring some potential actual errors but I think it's the best that can be done for now and the query validation should be catching the bulk of those.

[gmac]

@lucianjon – comment added! I had the same thought about that error line, the logic is weird and goes against the grain of exceptions being exceptional. For a refactor, I've sort of been thinking that the isBoundary map could become more robust with other useful type information, for example:

FederationObjects: {
  MyNamespace: { Kind: "namespace", Implementations: nil },
  MyBoundary: { Kind: "boundary", Implementations: nil },
  MyInterface: { Kind: "interface", Implementations: ["MyBoundary"] },
}

That kind of structure would be able to distinguish boundaries and namespaces, and would provide a pre-compiled cache of all abstract boundaries so that they wouldn't have to be enumerated repeatedly.

[lucianjon]

@lucianjon – comment added! I had the same thought about that error line, the logic is weird and goes against the grain of exceptions being exceptional. For a refactor, I've sort of been thinking that the isBoundary map could become more robust with other useful type information, for example:

FederationObjects: {
  MyNamespace: { Kind: "namespace", Implementations: nil },
  MyBoundary: { Kind: "boundary", Implementations: nil },
  MyInterface: { Kind: "interface", Implementations: ["MyBoundary"] },
}

That kind of structure would be able to distinguish boundaries and namespaces, and would provide a pre-compiled cache of all abstract boundaries so that they wouldn't have to be enumerated repeatedly.

Wicked cheers, will get this in. Yeah agreed on the refactor, we have access to all the information we need when we merge schemas and could easily just keep it all stored on there. Can write an issue up for it or if you'd like to, go for it!

[gmac]

@lucianjon – https://github.com/movio/bramble/issues/113