Skip to content

Conversation

@stephenplusplus
Copy link
Contributor

Fixes #1908

Previously, we took an input object that would go to request and iterated over all of the properties. This turned out to be an issue when the request object had properties containing circular references. What we really want to do is loop a select number of properties which may include the projectId token:

  • reqOpts.qs (an object which gets serialized into a query string)
  • reqOpts.json (the JSON payload)
  • reqOpts.uri (the URL the request is sent to, which always exists)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 17, 2017
@callmehiphop
Copy link
Contributor

LGTM - my only request would be to add a (probably system) test that passes in something with a circular reference.

@stephenplusplus
Copy link
Contributor Author

stephenplusplus commented Mar 21, 2017

That's a fair request, but hopefully you can help me figure out the right way to handle this, because it might be more complex than it seems at first glance.

The error reported from #1908 is indeed from a recursion issue. And this PR does fix it, but not by handling the circular reference directly, but by narrowing the scope of the input to these properties that we loop over. In effect, that addresses the circular reference, because we aren't looping over unknown values on the reqOpts object, which seemed to be affected by the environment. This will now only loop over the parts of the request that could contain the project ID token, which we control.

But, the project ID token could be alongside user input, and that user input could have a circular reference, and when we iterate over it looking for the project ID token, the code would blow up the same way as in #1908. The complicated part is, each API-- or even, each method-- could have its own preferred way of handling circular references-- maybe it's okay, maybe it should be replaced with [Circular], maybe it should be removed, maybe it should blow up the code (granted, with a more helpful error, which will happen after #2068).

So, the question is-- do we handle circular references, and if so, how?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants