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

Use a single string for template ids #3647

Closed
hurryabit opened this issue Nov 27, 2019 · 9 comments · Fixed by #3991
Closed

Use a single string for template ids #3647

hurryabit opened this issue Nov 27, 2019 · 9 comments · Fixed by #3991
Assignees
Labels
component/json-api HTTP JSON API discussion Things to be discussed and decided team/ledger-clients Related to the Ledger Clients team's components.

Comments

@hurryabit
Copy link
Contributor

Currently, the template ids have the type

{ packageId: string; moduleName: string; entityName: string; }

where the packageId is optional when you send a request to the API but is present in all responses.

I propose we replace this by a single string of the format

<packageId>:<moduleName>:<entityName>
<moduleName>:<entityName>

depending on whether the package id is present or not.

Using strings instead of objects has at least two advantages:

  1. template ids can be used as keys in JavaScript objects
  2. template ids can be used as keys for discriminated unions, as described in https://www.typescriptlang.org/docs/handbook/advanced-types.html#discriminated-unions

The latter is particularly useful in a context where you want a single UI component to display contracts of different types. This happens, for instance, during upgrading when your ledger has only been partially upgraded so far and you have contract instances of the old and the new template you need to show in a UI.

Using colons as the separator is safe since we don't allow them in identifiers, see the DAML-LF spec of identifiers. The spec also uses colons as separators between package ids, module names and entity names.

In case you actually need the individual components of a template id you received from the JSON API, you could now get them via

  const [packageId, moduleName, entityName] = str.split(":");
@hurryabit hurryabit added the component/json-api HTTP JSON API label Nov 27, 2019
@hurryabit hurryabit added the discussion Things to be discussed and decided label Nov 27, 2019
@da-tanabe da-tanabe changed the title JSO API: Use a single string for template ids JSON API: Use a single string for template ids Nov 27, 2019
@leo-da leo-da added this to the HTTP JSON API Maintenance milestone Nov 27, 2019
@S11001001
Copy link
Contributor

I don't think we should do this, because we neatly avoid all mistake or disagreement of what an appropriate delimiter is by the current encoding, for now and the future. One need only browse the LFv1 spec history to see all sorts of cases where the rug was pulled from under anyone who thought their delimiter choice was safe. That's the reason we decided not to do this when originally designing the JSON API.

In case you actually need the individual components of a template id you received from the JSON API, you could now get them via

and likewise the inverse.

@hurryabit
Copy link
Contributor Author

and likewise the inverse.

That is not really true. I had types v1.Foo and v2.Foo in scope in the context of an upgrade and defined type Foo = v1.Foo | v2.Foo. If we follow my proposal, I can do

switch (foo) {
  case: v1.Foo.templateId: { ... }
  case v2.Foo.templateId: { ... }
}

and get exhaustiveness checking in TypeScript. If you need to mangle the templateId field through a function first, you don't get exhaustiveness checking. We also build a big hashmap containing all templates, indexed by their templateId, in the support library for the daml2ts codegen. Using strings as keys in a hashmap is way more pleasant than using objects.

I suppose you're referring to the fact that we used the dot as a delimiter between module and entity names. That was obviously not a smart idea and I already opposed it back then but didn't have the power to stop it from happening. The colon is not allowed as a symbol in a name component and never has been. We've actually used it as a delimiter in all contexts where we need a textual representation of a fully qualified identifier for quite some time now and it hasn't caused any issues. Thus, I don't see why the historic mistake should stop us from doing something that has proven itself new.

@hurryabit
Copy link
Contributor Author

Can we please keep this conversation going?

@S11001001
Copy link
Contributor

I can do

Would a reference to v1.Foo.templateId.packageId not have been an equally functional discriminator in this switch example?

I suppose you're referring to the fact that we used the dot as a delimiter between module and entity names.

I am referring to several extensions of allowed character sets as well.

@hurryabit
Copy link
Contributor Author

hurryabit commented Dec 12, 2019

I am referring to several extensions of allowed character sets as well.

I'm fine to extend the spec saying that colons are taboo. In fact, we don't need to add any more characters since we now have an escaping mechanism.

Would a reference to v1.Foo.templateId.packageId not have been an equally functional discriminator in this switch example?

In this particular example, I could have used the package id as the discriminator. However, what I would like to do in general, is to write something like the getOwner function in the code snippet below. This will become very ugly if your discriminator is a triple of strings.

// Interface of the TypeScript bindings library:
interface Template<T> {
    templateId: string;
    validate: (payload: T) => boolean;
}

interface HasTemplateId<I> {
  templateId: I;
}

type Contract<T> = {
    templateId: string;
    contractId: string;
    payload: T;
}

const fetchAll = <T, I>(template: Template<T> & HasTemplateId<I>): (Contract<T> & HasTemplateId<I>)[] => {
    throw Error(`fetchAll for ${template} not yet implemented.`)
}

// Generated by the DAML to TypeScript codegen:
type Foo = {
    fooOwner: string;
}
const Foo: Template<Foo> & HasTemplateId<'pkg1:X:Foo'> = {
    templateId: 'pkg1:X:Foo',
    validate: (_) => true,
}

type Bar = {
    barOwner: string;
}
const Bar: Template<Bar> & HasTemplateId<'pkg2:Y:Bar'> = {
    templateId: 'pkg2:Y:Bar',
    validate: (_) => true,
}

// User written code:
type FooBarContract =
    (Contract<Foo> & HasTemplateId<typeof Foo.templateId>) |
    (Contract<Bar> & HasTemplateId<typeof Bar.templateId>);

const fetchWorld = (): FooBarContract[] => {
    const foos: FooBarContract[] = fetchAll(Foo);
    const bars: FooBarContract[] = fetchAll(Bar);
    return foos.concat(bars);
}

const getOwner = (fooBar: FooBarContract): string => {
    switch (fooBar.templateId) {
        case Foo.templateId: return fooBar.payload.fooOwner;
        case Bar.templateId: return fooBar.payload.barOwner;
    }
}

@leo-da
Copy link
Contributor

leo-da commented Dec 13, 2019

I personally prefer a structured templateId, the way we have it defined right now. We have a strongly typed value (as strong as we can get in JSON), I don't think switching to a plain string templateId makes sense.

I also get what @hurryabit is going after. However, TypeScript is not the only language we are targeting with the JSON API.

@hurryabit: DDD is the answer. You already defined the domain model the way you want it, the way it fits your specific domain. Handle the conversion as part of the JSON serialization/deserialization and you can have your plain string templateId. JSON is just a transport, so treat it that way.

@leo-da
Copy link
Contributor

leo-da commented Dec 13, 2019

If type guard is the only reason you wanted to use string for Template ID.... I found that you can use a composite value (record) as a type guard and as a singleton type (it does not have to be primitive). Here is my experiment below.

// Interface of the TypeScript bindings library:

interface TemplateIdT {
  packageId: string;
  moduleName: string;
  entityName: string;
}

const FooTemplateId: TemplateIdT = {
  packageId: "pkg1",
  moduleName: "X",
  entityName: "Foo"
};

const BarTemplateId: TemplateIdT = {
  packageId: "pkg2",
  moduleName: "Y",
  entityName: "Bar"
};

enum TemplateId {
  FooTemplateId,
  BarTemplateId
}

interface Template<T> {
  templateId: TemplateId;
  validate: (payload: T) => boolean;
}

interface HasTemplateId<I> {
  templateId: I;
}

type Contract<T> = {
  templateId: TemplateId;
  contractId: string;
  payload: T;
};

const fetchAll = <T, I>(
  template: Template<T> & HasTemplateId<I>
): (Contract<T> & HasTemplateId<I>)[] => {
  throw Error(`fetchAll for ${template} not yet implemented.`);
};

// Generated by the DAML to TypeScript codegen:

type Foo = {
  fooOwner: string;
};

const Foo: Template<Foo> & HasTemplateId<TemplateId.FooTemplateId> = {
  templateId: TemplateId.FooTemplateId,
  validate: _ => true
};

type Bar = {
  barOwner: string;
};

const Bar: Template<Bar> & HasTemplateId<TemplateId.BarTemplateId> = {
  templateId: TemplateId.BarTemplateId,
  validate: _ => true
};

// User written code:
type FooBarContract =
  | (Contract<Foo> & HasTemplateId<TemplateId.FooTemplateId>)
  | (Contract<Bar> & HasTemplateId<TemplateId.BarTemplateId>);

const fetchWorld = (): FooBarContract[] => {
  const foos: FooBarContract[] = fetchAll(Foo);
  const bars: FooBarContract[] = fetchAll(Bar);
  return foos.concat(bars);
};

const getOwner = (fooBar: FooBarContract): string => {
  switch (fooBar.templateId) {
    case TemplateId.FooTemplateId:
      return fooBar.payload.fooOwner;
    case TemplateId.BarTemplateId:
      return fooBar.payload.barOwner;
  }
};

@S11001001 S11001001 changed the title JSON API: Use a single string for template ids Use a single string for template ids Dec 16, 2019
@hurryabit
Copy link
Contributor Author

@leo-da Your example does not what you intended. In particular, the enum definition

enum TemplateId {
  FooTemplateId,
  BarTemplateId
}

does not define TemplateId.FooTemplateId to be the FooTemplateId you defined above but rather defines it to be the string 'FooTemplateId'. Thus, the definition of Foo.templateId in

const Foo: Template<Foo> & HasTemplateId<TemplateId.FooTemplateId> = {
  templateId: TemplateId.FooTemplateId,
  validate: _ => true
};

is not what the codegen would generate. In fact, it cannot generate something like this in an open world where we don't know all the templates in advance.

The only types for which you can do this pseudo dependently typed programming in TypeScript are strings and numbers. It does not work for objects. This is one of the main reasons why I would like to have template ids to be strings.

The other main reason is that using template ids as keys of a dictionary is otherwise unnecessarily hard since objects are compared by reference when used as keys of JavaScripts Map type, whereas strings are compared by value.

That said, I would like to move forward with that ticket. I'm totally aware that the approach I suggested has some dangers and I'll take responsibility if they bite us. However, we are making a trade-off between some potential problems, which I think won't bite us, and user experience. In this instance, I'm heavily leaning towards user experience and take a small risk, which we are in control of anyway.

@leo-da
Copy link
Contributor

leo-da commented Jan 7, 2020

@hurryabit OK. templaetId will be a string in the JSON.

@leo-da leo-da added the wip-issue This issue is being worked on. Use draft PRs for work in progress PRs. label Jan 8, 2020
leo-da added a commit that referenced this issue Jan 9, 2020
* test cases: domain.TemplateId JSON serialization to JsString

* JSON protocol updated

* Fixing json-api test cases

* test cases: domain.TemplateId JSON serialization to JsString

* JSON protocol updated

* Fixing json-api test cases

* Adapt daml2ts and support libraries

* Update documentation

CHANGELOG_BEGIN

[JSON API - Experimental]
- Use JSON string to encode template IDs. Use colon (``:``) to separate parts of the ID.
  The request format, with optional package ID:
  - "<module>:<entity>"
  - "<package ID>:<module>:<entity>"
  The response always contains fully qualified template ID in the format:
  - "<package ID>:<module>:<entity>"
  See #3647.

CHANGELOG_END

* Minor documentation formatting changes.

Co-authored-by: Martin Huschenbett <martin.huschenbett@posteo.me>
@leo-da leo-da removed the wip-issue This issue is being worked on. Use draft PRs for work in progress PRs. label Jan 9, 2020
@stefanobaghino-da stefanobaghino-da added the team/ledger-clients Related to the Ledger Clients team's components. label Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/json-api HTTP JSON API discussion Things to be discussed and decided team/ledger-clients Related to the Ledger Clients team's components.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants