flatten models packages in Go to avoid unsupported circular references

[papegaaij]

For larger APIs, it is nice to put different parts of the model in different namespaces. For example, in our API, we've got over 40 types related to authentication and about 30 related to groups of users. In Java, we've put these classes in different subpackages, and I think this works well for other languages, such as C# as well. However, these types do have relations pointing to types in other subpackages, such as accounts being member of a group and the groups an account is member of. These dependencies often form cycles. This currently breaks code generation for Go, as Go does not allow circular dependencies between modules.

I'm not sure what the best way to fix this would be, but I guess the best solution would be to flatten the models modules in Go and generate one large module. I think cross-type relations are quite common in APIs and requiring the provider of the API to not use relations over namespaces does not seem viable to me. IMHO this should be implemented in the GoRefiner.

[baywet]

Thanks for reaching out on the topic.

The go refiner already has a step that removes properties in models that depends on sub-packages to avoid circular dependencies.

With the level of details you've provided, it sounds like your scenario should already be covered as working?
Can you maybe provide a mermaid diagram of your scenario?

[papegaaij]

My problem is with properties from sub-packages, referencing other sub-packages. I've created a very simple spec with a.a referencing b.b and vice versa. Removing properties would fix the the compilation issues on Go, but that would require removing all properties that reference sibling-packages. In our case, that would result in an SDK that is not very usable.

I think the only viable route would be to place all model classes in a single package. This would however give rise to some other issues, most notably duplicate type names. Perhaps, a solution would be to rename the types to include the sub-package name in the type name. I believe this is a common practice for other generators.

I can start working on a PR if you agree with this approach.

[baywet]

No I don't think flattening namespaces would be a viable solution, it'd result in conflicts (we have the case in Microsoft Graph).

Indeed the step "looks down" but doesn't look at siblings, or children of siblings. What I'm a bit worried about with a "naïve approach" of projecting properties against every property is that:

• it'll be slow.
• we'll miss any case with more than 1 "hop" in the loop.
• it's not going to be deterministic about which property gets removed (the code dom is not emitted in any specific order)

The better thing to do would be to implement a cycle detection algorithm which probably represents a significant undertaking at this point.
Once we have a list of loops, we need to come up with an algorithm that'll consistently "cut" the loop at the same place across iterations or even if the graph changes slightly.

[papegaaij]

Naive flattening would indeed result in conflicts, but a different strategy would be to rename the classes during the flattening. For example, if I take some names from the Graph API:

microsoft.graph.group -> Group
microsoft.graph.security.group -> SecurityGroup

This should prevent most conflicts. Any remaining conflicts could then be solved using a simple postfix with an increasing number (this will probably be very rare).

Removing properties that cause cycles in the dependency-graph is possible, but as you already said, it will be hard to do in a stable way, and even then, you are removing properties defined for a reason in the API. In my simple case with a.a pointing to b.b and vice versa, how would you decide which property to remove and how would the developer using the SDK know this happened and still set or read these properties?

In our API, we've got quite a few of these references to sibling packages and many of them are required properties. We often create a superclass to be a reference to another entity. So you have a.SubA extends a.BaseA and b.SubB extends b.BaseB with a.SubA containing a reference to b.BaseB and b.SubB containing a reference to a.BaseA. Removing even a subset of these references would make the SDK useless for a developer. That would leave me with no other choice than to do a pre-processing step to update the OpenAPI spec for Go only to flatten the names to force Kiota to generate a single module anyway.

[baywet]

In your example, what do you expect to happen if microsoft.graph already contains SecurityGroup?

[papegaaij]

That's the big problem you always face when you are changing names, but I think a simple increasing number at the end should do the trick. So one of the types will be named SecurityGroup and the other one will be SecurityGroup0. This is not ideal, but IMHO far better than properties missing. Also, I think such collisions will not be very common.

[baywet]

Thanks for developing the thought to the end. The other major issue with moving the class is that it represents two breaking changes:

• If I used kiota v1.3 to generate a client, a property got trimmed, but I might still have taken a dependency on the model. Now with Kiota v1.x the model moves and my code is broken.
• If the description changes to add a property which results in a loop, now my model moves as I refresh the client.

Both of those go against our support policy and would require a major bump in the first case, and wouldn't have a solution in the second case.

Trimming properties, although not ideal, makes for a more stable generation result over time. What do you think?

[papegaaij]

My intention was to always entirely flatten the codespace of the models module. This would be stable with any regeneration of the client and be very predictable with changes to the OpenAPI spec. Golang being a stable language, this does indeed require a major release, which is unfortunate.

Trimming properties does not generate a stable API either. Adding a property somewhere, might cause some other property to be removed. The generator has no way of knowing which property existed in the old API. In my simple a.a + b.b example, if I start with only the reference from b.b to a.a, and add the reference from a.a to b.b later on, the first might get removed.

I'm not an experienced Golang developer, but my search so far seems to suggest that putting your entire model in one package is the preferred way of doing it in Golang. I've found several large SDKs which use this approach.

[baywet]

The current implementation is deterministic in the sense that it removes properties that "point down". But yes, I agree that coming up with something deterministic for a broader loops context over time will be challenging at best.

Here are two discussions I've been part of which are relevant to this issue:

The first one is at the API council at Microsoft while working on the namespacing API pattern guideline. Long story short we ended up recommending that type names across namespaces DO NOT need to be unique, because it'd defeat the purpose of namespacing. And we also recommended for client applications TO NOT flatten namespacing (although that's not part of the published guidance for some reason).

While working on Go generation in the early days, we faced performance issues with the Microsoft Graph Go SDK (generated from Kiota). And I reached out to people working on the Go compiler itself. Their feedback was along the lines of "don't make main packages (root + sub packages) that are so large", "subpackaging is supported, don't nest too deep", "break into multiple main packages if you can". Obviously, that's hard to conciliate with most REST APIs that usually make use of namespacing, have a large number of inter-related models spanning across packages.

I understand the Go community made early design choices to keep things as simple as possible. But my opinion is that it's reached a point where some of those choices are non-practical in a large number of scenarios. (the other example I have in mind being how generics work...). API producers have come to expect the client application will have a number of capabilities based on very basic language capabilities that are available today in the vast majority of programing languages out there.

Now, that additional context while useful doesn't solve your problem. But I don't think a solution where we flatten namespaces would work at this point. If we can come up with a deterministic way to detect loops and clip them deterministically it'd be a huge win. And it'd also go a long way to unblocking our ruby work.

[papegaaij]

I don't think we want this issue to be closed. This is a real issue with the current Go generation, that cannot be fixed before 2.0 and will require an explicit decision between either of these two options:

• The models package will be flattened to one large package entirely.
• Circular references between packages (either between siblings or from a sub package to one of its ancestors) will be explicitly not supported and all these references will be pruned.

[baywet]

That bot is driving by tags. And we use the tags to "force the conversation to a decision or to be closed" as you rightfully pointed out.
Flattening packages is not something we can implement before a v2. Microsoft Graph doesn't really have this issue, you've already built some pre-processing for your case. So now the question becomes "do we have enough demand that requires fixing this issue before v2?"
Unless you want to spend the time implementing a deterministic trimming of circular references, my suggestion here is to park this to v2 with the intention of flattening things out. And wait to see if we get additional demand that'd make it a more pressing matter. What do you think?

[papegaaij]

Yes, I agree. I think flattening is the go-way of doing it, but this can't be done before v2. Trimming is a lot of work and hard to impossible to get deterministic. Let's park this for v2.