feat: two-way type references #684

josephaxisa · 2021-05-14T10:16:47Z

Modified @looker/sdk-codegen such that given a type such as DashboardElement, it records its parents DashboardElement[] and Dashboard.

Also wired this up in @looker/api-explorer:

Given a child type such as DashboardElement, record its parents DashboardElement[] and Dashboard in parentTypes

github-actions · 2021-05-14T10:24:50Z

Codegen Tests

  1 files   6 suites 21s ⏱️
58 tests 46 ✔️ 12 💤 0 ❌

Results for commit c5daa46.

github-actions · 2021-05-14T11:15:41Z

Codegen Tests

  1 files   6 suites 20s ⏱️
58 tests 46 ✔️ 12 💤 0 ❌

Results for commit 5d77aed.

github-actions · 2021-05-14T11:16:11Z

APIX Tests

    1 files ±0   72 suites ±0 2m 40s ⏱️ +21s
268 tests ±0 256 ✔️ ±0 12 💤 ±0 0 ❌ ±0
287 runs ±0 275 ✔️ ±0 12 💤 ±0 0 ❌ ±0

Results for commit 5d77aed. ± Comparison against base commit 952ed30.

joeldodge79 · 2021-05-14T12:41:11Z

packages/sdk-codegen/src/sdkModels.ts

+          propType.elementType?.parentTypes.add(propType.name)
+          propType.elementType?.parentTypes.add(this.name)


why do we want both DashboardElement[] and Dashboard? It seems to me that IType.{types|parentTypes} is meant to reference other IType objects which I believe (though I could be wrong) are anything that would be in a "$ref" value from the spec. I don't think DashboardElement[] satisfies that requirement. In fact, in APIX you're using typeRefs(...) which appears to filter out anything that isn't a "$ref" (and thus DashboardElement[] is removed)?

I don't know which of these two lines puts in DashboardElement[] but I think it should be removed.

Caveat: not super up to speed on the actual details/usage of the SdkModel so I could be totally wrong here.

parentTypes is primarily for APIX explorer documentation purposes to link a type to the types that use or "contain" it. For exploration, we don't bother to show an array collection type, just the underlying complex type.

This is a good point and would only require some additional validation in sdkModels.ts. I'm unsure whether we should do that or not though, I'll have to defer to @jkaster

I didn't answer completely. I think DashboardElement should have parent types that are Dashboard and DashboardElement[] because both "contain" DashboardElement.

I guess my more basic question is what what is a "type" in the context here.

this is an instance of Type that implements IType

IType.{types|parentTypes}is currently just Set<string> but it seems to me like they're more meant to be Set<IType> (which I know doesn't actually work under the hood due to object equality checks on a set).

If that's correct (i.e. type/parentTypes is mean to represent a collection of IType) then what is an IType? A Dashboard is an IType but DashboardElement[] is not. The former exists as a value to a $ref key in the schema where as the latter never does, rather DashboardElement is referenced in the items key of another IType's property that has a type value of "array"?

On the other hand, maybe types/parentTypes is not a loose reference to IType but rather meant to be the union of IType and IType[]? But, as you say it's primarily for APIX which promptly discards the IType[] type by calling typeRefs(...)?

Or maybe I'm just up too early and my brain isn't on :-) happy to hash out in person later

Ok, I thought that might be the question. They must be Set<string> rather than IType to prevent circular references that break JSON serialization. You'll find we use the name of the type rather than the actual Type in several collections for this reason.

I'm not doing a good job expressing my question/concern: it's not about the implementation of the typescript type or javascript runtime implementation of IType.{types|parentTypes}: my question/concern is what kinds of "things" do IType.{types|parentTypes} refer to. I believe they refer to IType things (even though there's no typescript type constraint because we're just using Set<string>), not container-datastructures of IType things and hence DashboardElement[] does not belong.

I'm thinking of an ERD - a parentType would be another box but would not also include the many-to-one line-arrow pointing to that box

I guess I see DashboardElement[] and Dashboard as two fundamentally different kinds of things:

Dashboard - a real thing (a User is a real thing etc)
DashboardElement[] - a programming construct to describe a collection of real things

Housing those under the same property feels like saying "one of my parents is named Tom, and I have another parent whose name is 'Tom's kids'"

I agree it gets muddy. What do you suggest instead? We definitely need to link child types of a collection to the user of said collection. For visual representation in APIX, this works. Can we create a ticket to work out this design and address it in another PR, because I've already thought about this and haven't come up with anything clearly better yet, and for the purpose of APIX type ref navigation this works.

We definitely need to link child types of a collection to the user of said collection

I don't totally understand what this means w/out seeing concrete code but I'll take your word for it: carry on :-)

joeldodge79 · 2021-05-14T12:43:19Z

packages/sdk-codegen/src/sdkModels.spec.ts

@@ -732,6 +732,18 @@ describe('sdkModels', () => {

  describe('method and type xrefs', () => {
    describe('custom types', () => {
+      it('determines parent type of array types', () => {


maybe add a test for "parent of hash types" since your fix in sdkModels is

propType.instanceOf('ArrayType') || propType.instanceOf('HashType')

(or conversely remove the || HashType condition)

We have no complex hash types in the model right now, so we need to create a sample spec for that.

I didn't add a test for this because unfortunately we do not have any typed HashType in the spec. All parameters of type IDictionary expect a scalar property value. I added the check because I expect it to work when we do, but perhaps I should remove it so that when we do need it, we'll remember to add a test

we've modified test/openApiRef.json in the past to add spec schema that we don't already have in Looker: 1b64e1b#diff-90f6053ccc4c15adb9cbbba6833dbfb5a6206c6fb3793fe206f10f71230b73faR29331

So yeah, I'd say either remove the check (perhaps adding a TODO) or add the test

added the test

updated test/openApiRef.json with a fake schema for this

jkaster

Looks good.

github-actions · 2021-05-14T16:05:37Z

APIX Tests

    1 files   72 suites 2m 11s ⏱️
268 tests 256 ✔️ 12 💤 0 ❌
287 runs 275 ✔️ 12 💤 0 ❌

Results for commit 6119e97.

github-actions · 2021-05-14T16:06:59Z

Codegen Tests

  1 files   6 suites 26s ⏱️
58 tests 46 ✔️ 12 💤 0 ❌

Results for commit 8f35b37.

github-actions · 2021-05-14T16:23:34Z

Codegen Tests

  1 files ±0   6 suites ±0 24s ⏱️ ±0s
58 tests ±0 46 ✔️ ±0 12 💤 ±0 0 ❌ ±0

Results for commit 6119e97. ± Comparison against base commit 8c75bba.

joeldodge79 · 2021-05-14T16:55:31Z

packages/sdk-codegen/src/sdkModels.spec.ts

+      it('determines parent type of complex hash types', () => {
+        const hashProp = apiTestModel.types.ComplexHashProp
+        expect(hashProp.parentTypes).toEqual(
+          new Set(['Hash[ComplexHashProp]', 'ComplexHash'])
+        )
+      })
+


thanks for adding the test. I'll shut up now but it still smells like something is wrong in our modeling when 'Hash[ComplexHashProp]', 'ComplexHash' are elements of the same property

feat: add two-way type references

d253a1d

Given a child type such as DashboardElement, record its parents DashboardElement[] and Dashboard in parentTypes

google-cla bot added the cla: yes label May 14, 2021

josephaxisa requested review from joeldodge79 and jkaster May 14, 2021 10:16

Merge branch 'main' into jax/two-way-type-refs

c5daa46

feat: show two-way type refs in api explorer

5d77aed

jkaster previously approved these changes May 14, 2021

View reviewed changes

joeldodge79 suggested changes May 14, 2021

View reviewed changes

test for complex hash types

8f35b37

updated test/openApiRef.json with a fake schema for this

josephaxisa dismissed jkaster’s stale review via 8f35b37 May 14, 2021 14:16

josephaxisa requested review from joeldodge79 and jkaster May 14, 2021 14:17

Merge branch 'main' into jax/two-way-type-refs

6119e97

jkaster approved these changes May 14, 2021

View reviewed changes

joeldodge79 approved these changes May 14, 2021

View reviewed changes

josephaxisa merged commit fbc0565 into main May 14, 2021

josephaxisa deleted the jax/two-way-type-refs branch May 14, 2021 17:22

This comment has been minimized.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: two-way type references #684

feat: two-way type references #684

josephaxisa commented May 14, 2021 •

edited

Loading

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

joeldodge79 May 14, 2021

jkaster May 14, 2021

josephaxisa May 14, 2021

jkaster May 14, 2021

joeldodge79 May 14, 2021 •

edited

Loading

jkaster May 14, 2021

joeldodge79 May 14, 2021 •

edited

Loading

joeldodge79 May 14, 2021

jkaster May 14, 2021

joeldodge79 May 14, 2021

joeldodge79 May 14, 2021

jkaster May 14, 2021

josephaxisa May 14, 2021

joeldodge79 May 14, 2021

josephaxisa May 14, 2021

jkaster left a comment

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

github-actions bot commented May 14, 2021

joeldodge79 May 14, 2021

This comment has been minimized.

This comment has been minimized.

		propType.elementType?.parentTypes.add(propType.name)
		propType.elementType?.parentTypes.add(this.name)

feat: two-way type references #684

feat: two-way type references #684

Conversation

josephaxisa commented May 14, 2021 • edited Loading

github-actions bot commented May 14, 2021

Codegen Tests

github-actions bot commented May 14, 2021

Codegen Tests

github-actions bot commented May 14, 2021

APIX Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaster left a comment

Choose a reason for hiding this comment

github-actions bot commented May 14, 2021

APIX Tests

github-actions bot commented May 14, 2021

Codegen Tests

github-actions bot commented May 14, 2021

Codegen Tests

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

josephaxisa commented May 14, 2021 •

edited

Loading

joeldodge79 May 14, 2021 •

edited

Loading

joeldodge79 May 14, 2021 •

edited

Loading