Repository navigation
GraphFragmentCodec: read properties without reflection, refuse what it cannot write - #23
Merged
Merged
Conversation
…t cannot write encode read the properties of any node that is not a DetachedNodeGeneric by reflection and wrote the node with no properties when that failed. In atom's native image it always fails for cpg2's generated New* nodes, so a native build cached fragments holding only node labels, and the next run built its graph from them (AppThreat/atom#286, issue #21). On the JVM it fails for a class reflection may not access. Scala collection values were iterated by reflection too, and failed in a native image with an exception rather than an empty result. encode now reads a DetachedNodeGeneric's key-values directly and every other node through a PropertyReader the caller passes, once per node. A node the reader does not know, a value with no encoding (including a Scala collection, which the reader converts to java.util.List), a property pointing outside the fragment, and malformed key-values all make encode return empty, as it already did for diffs that are not add-only or self-contained. Nothing is written lossily and nothing throws, and the codec behaves the same on the JVM and in a native image. The three-argument encode is kept for diffs of generic nodes. GraphFragmentCodecPropertiesTest covers each of these cases, including the reproduction from the issue. Without the change, all ten cases that compile against the old API fail: four write a fragment that lost the node's properties, two write one that is right only because reflection works on the JVM, and four throw. Signed-off-by: Team AppThreat <cloud@appthreat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #21.
GraphFragmentCodec.encoderead the properties of any node that is not aDetachedNodeGenericby reflection, and wrote the node with no properties when that failed. In atom's native image it always fails for cpg2's generatedNew*nodes, so native builds cached fragments holding only node labels and built later graphs from them (AppThreat/atom#286). Scala collection values were also iterated by reflection, which throws in a native image.I triaged it with fixtures in
GraphFragmentCodecPropertiesTest. On main, four cases write a fragment whose node lost its properties: the private class from the issue, a node class withoutproperties(), one whoseproperties()throws, and such a node reached only as an edge endpoint. Two more are right only because reflection works on the JVM. The other four throw instead of returning empty: an unsupported value type, an unsupported edge property, a property pointing outside the fragment, and malformed key-values.The codec now uses no reflection at all. It reads a
DetachedNodeGeneric's key-values directly and every other node through aPropertyReaderthe caller passes, once per node. Anything it cannot write exactly makesencodereturn empty, the same contract it already had for diffs that are not add-only or self-contained. A caller then recomputes instead of caching a lossy fragment, and the JVM and a native image behave the same. The three-argumentencodestays for diffs of generic nodes. Scala collection values are no longer accepted; a reader for Scala nodes converts them tojava.util.List.I checked it end to end with native images built with GraalVM CE 25.0.2. Each run went cold (writing fragments), then warm (reading them back), then
--cache nonefor reference. With chen main and odb2 main, the warm native run on a Python project (DefectDojo'sdojo/tools) spun at full CPU until I stopped it after 575 s. On libexpat it exited 0 with 530 slices, none of which matched the 727 from the uncached run. With this change plus a chen reader forNew*nodes, the warm runs give byte-identical slices on the Python and JS projects, on the JVM and natively. C warm runs differ in about 62 of 727 slices both before and after this change, on the JVM too; that is a separate, pre-existing issue in C fragment restore.chen needs a matching change before it picks this up. Its
AstFragmentmust pass a reader for cpg2'sNew*nodes, or nothing gets cached. I have that on a chen branch with a cache-key revision, so fragments that earlier native builds wrote are not read again. It replaces the diff copy in AppThreat/chen#223 and goes up once this is released through cpg2. The version stays 3.1.1, unreleased, alongside #22. All tests pass withtestFull: 102 JUnit tests and 185 ScalaTest tests.