Skip to content
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

missing UI connection from Api to Bucket #6458

Closed
ekeren opened this issue May 12, 2024 · 6 comments · Fixed by #6509
Closed

missing UI connection from Api to Bucket #6458

ekeren opened this issue May 12, 2024 · 6 comments · Fixed by #6509
Assignees
Labels
🐛 bug Something isn't working 🛫 console Console

Comments

@ekeren
Copy link
Collaborator

ekeren commented May 12, 2024

I tried this:

bring cloud;

let api = new cloud.Api();
let b = new cloud.Bucket();
api.post("/doc/:id", inflight (req) => {
  b.put(req.vars.get("id"), req.body! );
});

This happened:

image

I expected this:

To see connection between api gateway to bucket

Is there a workaround?

No response

Anything else?

No response

Wing Version

0.73.41

Node.js Version

No response

Platform(s)

No response

Community Notes

  • Please vote by adding a 👍 reaction to the issue to help us prioritize.
  • If you are interested to work on this issue, please leave a comment.
@ekeren ekeren added the 🐛 bug Something isn't working label May 12, 2024
@ekeren ekeren changed the title can't see connection from Api to Bucket missing UI connection from Api to Bucket May 12, 2024
@ekeren ekeren added the 🛫 console Console label May 12, 2024
@skyrpex
Copy link
Contributor

skyrpex commented May 13, 2024

We're missing information from the simulator. These are the connections listed:

[
	{
		"source": "root/Default/Api",
		"target": "root/Default/Api/OnRequestHandler0",
		"name": "post()"
	},
	{
		"source": "root/Default/$Closure1_0",
		"sourceOp": "handle",
		"target": "root/Default/Bucket",
		"targetOp": "put",
		"name": "put"
	}
]

So basically we're missing some connection from root/Default/Api/OnRequestHandler0 to root/Default/$Closure1_0.

@Chriscbr any insights?

@Chriscbr
Copy link
Contributor

Chriscbr commented May 13, 2024

I did a bit of digging. In theory there should be another connection getting generated, but there's a little architectural issue:

  1. Connection data is currently associated with the app (construct tree). I think this is probably a good thing(?) - if you're unit testing apps and you create multiple apps aka construct trees, their connection data is separate as you'd expect. We also have other data associated with the construct tree, like platform parameters.
  2. Inflights created with the inflight() function here were changed in feat(sdk): inflights are not required to be resources #4993 to no longer be constructs, so they don't automatically have any access to the app or construct tree. This means there's no way for an inflight to add info to the tree about its connection data. This limitation was only really uncovered when we started using the inflight() function within the SDK's source code in chore(sdk): deprecate makeHandler() and convertBetweenHandlers() #6354.

I feel like the best solution here would be to make inflights constructs again. It would also let us clean up this issue where token resolvers are globals, which seems like it might cause issues in the future. But I'm curious what y'all think @MarkMcCulloh @eladb

@MarkMcCulloh
Copy link
Contributor

there's no way for an inflight to add info to the tree about its connection data

Could the inflight host (e.g. OnRequestHandler) add the connection itself? It knows what connections the inflight has (lifts) so maybe it can link itself to them.
Inflights themselves are just some data that happens to relate itself to the construct tree. It would be like representing arbitrary strings as constructs just because could have a possible relationship to the tree (via tokens).
I wouldn't die on this hill but philosophically, non-construct inflights make more sense to me.

@Chriscbr
Copy link
Contributor

Chriscbr commented May 14, 2024

note: It looks the issue of broken extension also applies whenever you create an inflight using the TypeScript framework:

import { cloud, inflight, lift, main } from "@wingcloud/framework";
import assert from "node:assert";

main((root, test) => {
  const bucket = new cloud.Bucket(root, "Bucket");
  const fn = new cloud.Function(
    root,
    "Function",
    lift({ bucket }).inflight(async (ctx) => {
      ctx.bucket.put("key", "value");
      return "hello, world";
    })
  );

  test(
    "fn returns hello",
    lift({ fn }).inflight(async ({ fn }) => {
      assert.equal(await fn.invoke(""), "hello, world");
    })
  );
});
Screenshot 2024-05-14 at 1 14 03 PM

With the same code in Wing it works:

bring cloud;

let bucket = new cloud.Bucket();
let fn = new cloud.Function(inflight () => {
  bucket.put("key", "value");
  return "hello, world";
});

test "fn returns hello" {
  assert(fn.invoke() == "hello, world");
}
Screenshot 2024-05-14 at 1 16 33 PM

Could the inflight host (e.g. OnRequestHandler) add the connection itself? It knows what connections the inflight has (lifts) so maybe it can link itself to them.

I'd like to take you up on this suggestion. Putting it another way, we'd be making it the responsibility of the inflight host to register any connection data while it's lifting the inflight. It feels like a logical place to do the work. It would mean that a free-floating inflight closure (that doesn't get used anywhere) wouldn't emit any connection data, which also feels right. I imagine the logic can be included as part of the lifting stuff here:

// indicates that we are calling the inflight constructor and the
// inflight "handle" method on the handler resource.
Lifting.lift(this.handler, this, ["handle"]);

@ekeren
Copy link
Collaborator Author

ekeren commented May 16, 2024

Just making sure we are prioritizing this issue, currently even our classic getting started example looks "broken"

image

@mergify mergify bot closed this as completed in #6509 May 17, 2024
mergify bot pushed a commit that referenced this issue May 17, 2024
Fixes #6458

Some UI connections were missing in the Wing Console because when generating information about different resource nodes, inflight closures created in Winglang were treated as resources while inflight closures created using the TypeScript framework were not. To fix this discrepancy, this PR changes how connections are generated so that all intermediate inflight closures are no longer modeled as nodes. Instead, inflight closures (both those created in Winglang and in TypeScript) are explored until resource-like nodes are reached.

Taking this app as an example:

```js
let b = new cloud.Bucket();

let listData = inflight () => {
    b.list();
};

new cloud.Function(inflight () => {
    listData();
    b.put("hello.txt", "world");
});
```

Previously the SDK generated this connection data:

```js
{
  "source": "root/Default/$Closure1_0",
  "sourceOp": "handle",
  "target": "root/Default/Bucket",
  "targetOp": "list",
  "name": "list"
},
{
  "source": "root/Default/$Closure2_0",
  "sourceOp": "handle",
  "target": "root/Default/Bucket",
  "targetOp": "put",
  "name": "put"
},
{
  "source": "root/Default/$Closure2_0",
  "sourceOp": "handle",
  "target": "root/Default/$Closure1_0",
  "targetOp": "handle",
  "name": "handle"
},
{
  "source": "root/Default/Function",
  "sourceOp": "invoke",
  "target": "root/Default/$Closure2_0",
  "targetOp": "handle",
  "name": "handle"
},
{
  "source": "root/Default/Function",
  "sourceOp": "invokeAsync",
  "target": "root/Default/$Closure2_0",
  "targetOp": "handle",
  "name": "handle"
}
```

Now it generates this connection data (note that all "closure" nodes are gone - we just emit relationships between real resources):

```js
{
  "source": "root/Default/Function",
  "sourceOp": "invoke",
  "target": "root/Default/Bucket",
  "targetOp": "put",
  "name": "put"
},
{
  "source": "root/Default/Function",
  "sourceOp": "invoke",
  "target": "root/Default/Bucket",
  "targetOp": "list",
  "name": "list"
},
{
  "source": "root/Default/Function",
  "sourceOp": "invokeAsync",
  "target": "root/Default/Bucket",
  "targetOp": "put",
  "name": "put"
},
{
  "source": "root/Default/Function",
  "sourceOp": "invokeAsync",
  "target": "root/Default/Bucket",
  "targetOp": "list",
  "name": "list"
}
```

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [ ] Docs updated (only required for features)
- [ ] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
@monadabot
Copy link
Contributor

Congrats! 🚀 This was released in Wing 0.73.52.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 🛫 console Console
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants