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

TypeScript streamFetchByKey should support multiple {template, key} pairs per request #7034

Closed
leo-da opened this issue Aug 5, 2020 · 3 comments
Assignees
Labels
component/json-api HTTP JSON API team/ledger-clients Related to the Ledger Clients team's components.

Comments

@leo-da
Copy link
Contributor

leo-da commented Aug 5, 2020

JSON API supports multiple template, key pairs per /v1/stream/fetch request. See the doc: https://docs.daml.com/json-api/index.html#fetch-by-key-contracts-stream

[
    {"templateId": "<template ID 1>", "key": <key 1>},
    {"templateId": "<template ID 2>", "key": <key 2>},
    ...
    {"templateId": "<template ID N>", "key": <key N>}
]

However TS bindings' streamFetchByKey is defined like this:

streamFetchByKey<T extends object, K, I extends string>(
    template: Template<T, K, I>,
    key: K,
  ): Stream<T, K, I, CreateEvent<T, K, I> | null> {

You are actually wrapping the pair in the array:

const request = [{templateId: template.templateId, key}];

ACs

  • API user can specify multiple templateId, key pairs per request
@leo-da leo-da added this to the Language milestone Aug 5, 2020
@cocreature cocreature added the team/ledger-clients Related to the Ledger Clients team's components. label Aug 5, 2020
@cocreature cocreature removed this from the Language milestone Aug 5, 2020
@garyverhaegen-da garyverhaegen-da self-assigned this Aug 6, 2020
garyverhaegen-da added a commit that referenced this issue Aug 7, 2020
This is an attempt to address #7034 & #7036. I'd like to gather feedback
before moving further, though, because while I believe (based on limited
problem domain knowledge) that this is the rigth way to address them,
strictly speaking this does not fulfill their stated acceptance
criteria.

CHANGELOG_BEGIN

deprecation warning & upgrade instructions to be fleshed out

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 7, 2020
This is an attempt to address #7034 & #7036. I'd like to gather feedback
before moving further, though, because while I believe (based on limited
problem domain knowledge) that this is the rigth way to address them,
strictly speaking this does not fulfill their stated acceptance
criteria.

In particular, I have chosen to keep limiting both scopes to a single
template ID. Ignoring the fact that I'm having trouble reasoning about
the type of the resulting stream if we accept arbitrary lists of
template IDs in these calls, I'm also questioning whether we want to
encourage our users to build their applications on top of such
dynamically heterogeneous lists.

CHANGELOG_BEGIN

deprecation warning & upgrade instructions to be fleshed out

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 10, 2020
This is an attempt to address #7034 & #7036. I'd like to gather feedback
before moving further, though, because while I believe (based on limited
problem domain knowledge) that this is the rigth way to address them,
strictly speaking this does not fulfill their stated acceptance
criteria.

In particular, I have chosen to keep limiting both scopes to a single
template ID. Ignoring the fact that I'm having trouble reasoning about
the type of the resulting stream if we accept arbitrary lists of
template IDs in these calls, I'm also questioning whether we want to
encourage our users to build their applications on top of such
dynamically heterogeneous lists.

CHANGELOG_BEGIN

deprecation warning & upgrade instructions to be fleshed out

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 11, 2020
This is an attempt to address #7034 & #7036. I'd like to gather feedback
before moving further, though, because while I believe (based on limited
problem domain knowledge) that this is the rigth way to address them,
strictly speaking this does not fulfill their stated acceptance
criteria.

In particular, I have chosen to keep limiting both scopes to a single
template ID. Ignoring the fact that I'm having trouble reasoning about
the type of the resulting stream if we accept arbitrary lists of
template IDs in these calls, I'm also questioning whether we want to
encourage our users to build their applications on top of such
dynamically heterogeneous lists.

CHANGELOG_BEGIN

deprecation warning & upgrade instructions to be fleshed out

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 11, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

This also results in a somewhat confusing API because we end up with
overlapping functions: the new `streamQueries` can do everything that
the existing `streamQuery` can do, and the new `streamFetchByKeys` can
do everything that the existing `streamFetchByKey` can do, but we have
decided to keep the existing ones for now, to avoid breaking backwards
compatibility.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 11, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

This also results in a somewhat confusing API because we end up with
overlapping functions: the new `streamQueries` can do everything that
the existing `streamQuery` can do, and the new `streamFetchByKeys` can
do everything that the existing `streamFetchByKey` can do, but we have
decided to keep the existing ones for now, to avoid breaking backwards
compatibility.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 25, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the bototm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 25, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 25, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Aug 25, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Sep 8, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Sep 8, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Sep 8, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
garyverhaegen-da added a commit that referenced this issue Sep 8, 2020
This is an attempt to address #7034 & #7036. Strictly speaking, this
does not match their acceptance criteria, as this only supports multiple
queries, not mixed templates.

Because the two new functions can cover the exact same use cases (and
more) as the existing `streamQuery` and `streamFetchByKey`, the latter
are deprecated. The deprecation cycle I suggest is to deprecate them
immediately by annotating them as such in the documentation (done on
this PR).

That's it. I do not think we ever need to actually remove them, nor to
make them print annoying warnings or anything. There is nothing wrong
with the functions as they stand, they just don't fit in the API
anymore.

We could, at some point, move them to a separate documentation page, or
to the boottm of the existing one, but I feel even removing them from
the documentation is unnecessary.

CHANGELOG_BEGIN

- [JavaScript Client Libraries] Two new methods have been added to
  `daml-ledger`: `streamQueries` and `streamFetchByKeys`. They are
  similar to the existing singular versions, except they can take
  multiple queries and multiple keys, respectively, and return a union
  of what the corresponding individual queries/keys would have. Because
  these new functions can do everything the existing ones can, we are
  deprecating the latter, though there is no plan to remove them.

  Upgrade path is straightforward:

  ```
  streamQuery(t); => streamQueries(t, []);
  streamQuery(t, undefined); => streamQueries(t, []);
  streamQuery(t, q); => streamQueries(t, [q]);
  streamFetchByKey(t, k); => streamFetchByKey(t, [k]);
  ```

  There is one caveat, though: `streamFetchByKeys` is a little bit less
  lenient in the format in which it expects the key. If your existing
  code was conforming to the generated TypeScript code we provide,
  everything should keep working, but if you were using plain JS or
  bypassing the TS type system, it is possible that you used to
  construct keys that will no longer be accepted. The new function
  requires all keys to be given in the _output_ format of the JSON API,
  which is a little bit more strict than the general JSON <-> LF
  conversion rules.

CHANGELOG_END
@stefanobaghino-da
Copy link
Contributor

@garyverhaegen-da is this closed by #7066?

@stefanobaghino-da
Copy link
Contributor

@garyverhaegen-da I'll close this optimistically, please re-open if the issue has not been closed by the PR in the comment above.

@garyverhaegen-da
Copy link
Contributor

As explained in #7066, this issue is not fully covered: we can now pass multiple queries, but not multiple template IDs.

However, it does not look like anyone wants to champion the case for multiple template IDs anymore, so I'm happy to close this as "won't fix". Same goes for #7036.

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 team/ledger-clients Related to the Ledger Clients team's components.
Projects
None yet
Development

No branches or pull requests

4 participants