Skip to content

JS: Add backwards-compatible predicates to SocketIO #3262

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Apr 14, 2020

Makes the API exposed by SocketIO backwards compatible with clients from the 1.23 release.

There are no DIL changes so an evaluation shouldn't be necessary.

@asgerf asgerf added the JS label Apr 14, 2020
@asgerf asgerf added this to the 1.24 milestone Apr 14, 2020
@asgerf asgerf requested a review from erik-krogh April 14, 2020 15:00
@asgerf asgerf requested a review from a team as a code owner April 14, 2020 15:00
@erik-krogh erik-krogh self-assigned this Apr 14, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly fine. But you got a return-type wrong, and I think we can make the getAck predicates prettier.

(I found a spelling mistake: RecieveCallback needs to be ReceiveCallback, could you ninja-edit that into this PR?)

Comment on lines 289 to 290
result = getListener().getLastParameter() and
exists(result.getAnInvocation())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
result = getListener().getLastParameter() and
exists(result.getAnInvocation())
result.(ReceiveCallback).getReceiveNode() = this

And then add a getReceiveNode in ReceiveCallback.
You could change the return-type to ReceiveCallback?

And similar in the other getAck predicates.
(SendNode::getAck returns SendCallback).

SendNode getASender() { result.getAReceiver() = this }

/** Gets the acknowledgment callback, if any. */
DataFlow::FunctionNode getAck() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DataFlow::FunctionNode getAck() {
DataFlow::SourceNode getAck() {

The two getAck methods on the ReceiveNode classes returned a DataFlow::SourceNode in the previous API.

(The getAck methods on SendNode did return DataFlow::FunctionNode in the previous API, so those are fine).

(If you change the return-types to SendCallback/ReceiveCallback as suggested in the below, then that will ensure the correct return-type).

DataFlow::SourceNode getAReceivedItem() { result = getReceivedItem(_) }

/** Gets the acknowledgment callback, if any. */
DataFlow::FunctionNode getAck() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DataFlow::FunctionNode getAck() {
DataFlow::SourceNode getAck() {

This is the second getAck that returned a DataFlow::SourceNode in the previous API.

Comment on lines 86 to 91
/**
* DEPRECATED. Always returns `this` as a `ServerObject` now represents the origin of a server.
*
* Instead of `getOrigin()` to get a server origin from a reference, use `ServerObject.ref()` to get references to a given server.
*/
deprecated DataFlow::SourceNode getOrigin() { result = this }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The QLDoc is not quite accurate.

In the previous API the ServerObject class was of type TServer, and there was a one-to-one relation between TServer and a SourceNode origin.
The getOrigin method extracted the SourceNode origin from the TServer. (One can say the method converted between the two representations).
It didn't have anything to do with references like .ref().

(References are handled by the ServerNode class, and I kept that part of the API).

I think you can just remove the second line of the QLDoc, and then its fine.

@asgerf
Copy link
Contributor Author

asgerf commented Apr 15, 2020

Thanks. PTAL

@erik-krogh
Copy link
Contributor

Could you change the other 3 getAck methods to use SendCallback/ReceiveCallback?

erik-krogh
erik-krogh previously approved these changes Apr 20, 2020
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@semmle-qlci semmle-qlci merged commit 9fae953 into github:rc/1.24 Apr 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants