Skip to content

Fix: Spreading versioned models - #441

Merged
Timothee Guerin (timotheeguerin) merged 21 commits into
microsoft:mainfrom
timotheeguerin:fix/version-spread-generic
Apr 25, 2022
Merged

Fix: Spreading versioned models#441
Timothee Guerin (timotheeguerin) merged 21 commits into
microsoft:mainfrom
timotheeguerin:fix/version-spread-generic

Conversation

@timotheeguerin

@timotheeguerin Timothee Guerin (timotheeguerin) commented Apr 13, 2022

Copy link
Copy Markdown
Member

fix #244
fix #320

Problems

1. Using versioning in model expression and/or with spread

model Foo<T> {
   ...T
}

model Bar is Foo<{a: string, @added("2") b: string}>

Model expression didn't have a reference of which namespace they were defined in which prevent the verisoning lib to know which were the available version for those models

Library model that spread a model from another versioned namespace

@versioned("l1" | "l2")
  namespace VersionedLib {
    model Spread<T> {  ...T; }
  }

  @versioned("1" | "2")
  @versionedDependency(VersionedLib, {"1": "l1", "2": "l2"})
  namespace MyService {
    model Spreadable {
      a: int32;
      @added("2") b: int32;
    }
    model Test extends VersionedLib.Spread<Spreadable> {}
  }

The issue here is also that the property b is included in this instance VersionedLib.Spread<Spreadable> when running the projection "1" despite the property being added in version "2".

The problem here is that when the versioning projection run on VersionedLib.Spread<Spreadable> it is then using the version l1 as this is part of the VersionedLib scope.
Now when it comes time to resolve if property b should be included or not the version library check if the proeprty is available in version l1 instead of "1" which it of course cannot find and so assume the property should be included.

Solution

A tentative to fix spreading versioned model.

Changes:

  • Add a reference to the declaring namespace for model expressions: This allow using model expressions with versioning
  • When resolving the versions for a property look at the sourceProperty which allow. using spread of a versioned model and keep the versioning metadata
  • For a given projection keep a map of namespace to the projected version. This allows another library to spread another model.

Let me know what you think

Things to cleanup:

  • Passing map to projection argument is a bit hacky right now
  • Revert versioning sample
  • General cleanup

@timotheeguerin Timothee Guerin (timotheeguerin) changed the title Fix/version spread generic Fix: Spreading versioned models Apr 13, 2022
@bterlson

Copy link
Copy Markdown
Member

I feel like we need to support libraries which allow versioning in the context of their own versions (so if we have a dependency on D2 we spread only D2 properties) and spread in the context of local versions (so if we pass L1 to an abstraction, the properties that it adds are added in L1).

I think the only way this can work is if we pass versions as enum member references rather than strings, and this cleans up a lot of the logic around determining which versions are applicable. If we had such a mechanism, do you think this PR would be necessary or different?

@timotheeguerin

Timothee Guerin (timotheeguerin) commented Apr 13, 2022

Copy link
Copy Markdown
Member Author

Brian Terlson (@bterlson) I commented on this on the issue, I think it solve the problem 1 and would definitly cleanup a lot of code. But for the 2nd one the issue is not that we don't know what are the versions for the type but that we don't know which version we should use for that property which is why I made this version mapping.
Using the enum would help us to resolve what is the declaring namespace and what are the available versions but I don't think this help us in figuring out what in the current projection should be the version to use.

@azure-pipelines

Copy link
Copy Markdown

You can try these changes at https://cadlplayground.z22.web.core.windows.net/prs/441/

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

Labels

None yet

Projects

None yet

3 participants