lhr.stackPacks doesn't make the proto round trip

[brendankenny]

you'll see this if you run yarn update:sample-json on master. stackPacks will be deleted and then npx jest proto-test.js will fail.

Since CI only runs yarn diff:sample-json (just regenerating the original json, not the proto round-trip json), as long as stackPacks was manually added to sample_v2_round_trip.json, everything will be green on Travis :)

@exterkamp were the proto changes in #7243 not sufficient to get this automatically in the round trip result? I haven't been able to get good insight into why json_roundtrip_via_proto.py is dropping it.

[exterkamp]

So, it doesn't make the round trip in sample v2 because it is "stackPacks": [], which is the default value for repeated fields, and in the proto round trip processor we use the option including_default_value_fields=False which will reduce [] into nothing. So it works when a value exists, but not in the current sample json.
Example (samplev2 on left, round trip on right):

[exterkamp]

Preemptively closed this. This is a problem, but I don't know the obvious solution. Turning off including_default_value_fields=False still drops the empty repeated field. I'm not 100% how to get an empty repeated field to stick through the round trip. we can mod the proto preprocessor/tester to automatically add an empty array so that the test passes, but thats gross. We can add a test stack pack to sample v2 to make it non-empty?

[patrickhulce]

We can add a test stack pack to sample v2 to make it non-empty?

That seems like the most reasonable thing we care about :)

[brendankenny]

We can add a test stack pack to sample v2 to make it non-empty?

I mean, this was a test meant to catch when the LHR doesn't come back as expected when passed through the proto system. Seems like it's working cause that's what's happening here and it caught it :)

It's working in LR right now because the only code touching it treats it as if it were optional. It's only a matter of time until someone writes code that doesn't do that.

Seems like we need to do one of the following:

• we used ListValue for run_warnings to (apparently) deal with this. It does come through as [] in sample_v2_round_trip.json. Should we just match that?
• do something in the preprocessor to make it so the type that comes back matches the type we expect
• change the type we expect by making lhr.stackPacks optional instead of required

[exterkamp]

we used ListValue for run_warnings to (apparently) deal with this. It does come through as [] in sample_v2_round_trip.json. Should we just match that?

That would be less than ideal imo. We would lose all typing for stack-packs b/c they would just become Values at that point. I regret that we have to use ListValue for those and not a typed repeated field.

do something in the preprocessor to make it so the type that comes back matches the type we expect

This would be okay, but just kinda just punting the issue? Good for a short term solution.

change the type we expect by making lhr.stackPacks optional instead of required

I think this is my preference, since this would allow the proto to be as descriptive as possible and still pass the tests. But would this make the js less accurate?

[patrickhulce]

Just throwing this out there again, do we really need the proto to exactly match js representation without any transformations? It seems like having it go through multiple data formats is an inevitable recipe to require some sort of transformations/normalizations, and if we're sacrificing the useful of having it in a particular format (i.e. if we neuter proto's useful by erasing the useful types it provides), then why bother with proto at all? It might as well be a string that just gets parsed exactly as JSON.

Now that I've typed this all out I think I'm remembering that the answer here is that we have no ability to apply transformations to the JSON before it goes out into the world and so our PSI API customers would all need to be aware of these transformations in which case nevermind...

[exterkamp]

We do have some very limited transformations we can do. And I mean very limited. Like renaming. Or, null if doesn't exist, things like that. There is a possibility I could get this to work in Apiary. But that is a huge unknown and would be much more difficult than any other solution.