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

Should the contracts/search API error on unknown templates? #3771

Closed
bame-da opened this issue Dec 6, 2019 · 20 comments · Fixed by #4014
Closed

Should the contracts/search API error on unknown templates? #3771

bame-da opened this issue Dec 6, 2019 · 20 comments · Fixed by #4014
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

@bame-da
Copy link
Contributor

bame-da commented Dec 6, 2019

I see both pros and cons. Please discuss.

@bame-da bame-da added the discussion Things to be discussed and decided label Dec 6, 2019
@leo-da leo-da added this to the HTTP JSON API Maintenance milestone Dec 6, 2019
@bame-da
Copy link
Contributor Author

bame-da commented Dec 6, 2019

There seem to be two mental models:

  1. The API does pattern matching. Matching on an unknown pattern gives a runtime error
  2. The API does filtering. Since the %templates list is a disjunction (OR), adding an unknown item has no effect.

The argument for 1. is that it alerts users to mistakes.
The argument for 2. is that it's more lenient when it comes to upgrades or pruning. Clients don't need to know what exactly is deployed.

@leo-da
Copy link
Contributor

leo-da commented Dec 6, 2019

My preference would be option 1: Alert users. Let them fix the input.

We can also:

  • improve error reporting by introducing more granular error codes and providing error details in a more structured format (currently it is an array of strings), so it is easier to handle errors on the client side.
  • document JSON API error handling best practices

@S11001001
Copy link
Contributor

S11001001 commented Dec 10, 2019

Option 1 is already the model for the remainder of the query language, which is typed like the lf-value-json format; for example, if you specify "hi" in an int64 position, or a record field that is not part of that record, or a variant constructor that doesn't exist, you'll get an error, not an empty result set.

@hurryabit
Copy link
Contributor

There's at least one situation in which erroring out on a non-existent template is the wrong thing to do: Suppose you're upgrading an existing DAML model and hence need to support two versions of the model in your UI. At some point, all contracts of the old model will have been upgraded and you can garbage collect the old package from the ledger. The UI's queries to the JSON API would now start to fail for no good reason.

I suggest we solve the problem by adding a flag to the query endpoint of the JSON API which determines what happens if you query for a non-existent template. In strict mode, you'll get an error, in lenient mode, you don't. I have no strong preference which is the default, although I'm leaning slightly towards lenient mode.

@leo-da
Copy link
Contributor

leo-da commented Dec 11, 2019

The UI's queries to the JSON API would now start to fail for no good reason.

I thought we can have a client-side logic that automatically removes non-existing template IDs. See what I proposed above, JSON API could report archived/non-existing template IDs, which client HAS TO remove from the request (can be done automatically). If we do not do this, we will accumulate a bunch of obsolete/archived template IDs over time. Which JSON-API will keep validating/resolving and skipping, increasing the request-response latency.

In any case... to be honest we have a bigger problem. Which I think we have to address first.

if you upgrade a template and don't change an entity name (e.g. don't add a suffix IouV2), you will lose the ability to refer to a contract template by moduleName and entityName.

In other words if you have more than 1 template with the same moduleName:entityName JSON API will NOT be able to automatically resolve the fully qualified template ID, which is required for command submission. So you could not create, execute or search by moduleName:entityName.

@hurryabit
Copy link
Contributor

In any case... to be honest we have a bigger problem. Which I think we have to address first.

IMO, making the package ids optional was a good idea in the first place. I don't see a big problem here though because you can just adopt a style of always using package ids, which is what the daml2ts codegen does, for instance.

I thought we can have a client-side logic that automatically removes non-existing template IDs. See what I proposed above, JSON API could report archived/non-existing template IDs, which client HAS TO remove from the request (can be done automatically).

How would the client-side know which packages are there and which aren't? If if there was a way to find this out without changing the client-side, this would add significant complexity on the client-side, partially defeating the purpose of the JSON API.

If we do not do this, we will accumulate a bunch of obsolete/archived template IDs over time. Which JSON-API will keep validating/resolving and skipping, increasing the request-response latency.

I think it's fair to assume that users are aware that they pay runtime costs for what they put in their queries. If they care about performance, they can clean up their queries.

What would be the downside of making the behavior in case of non-existing template ids configurable? We can leave this out of the "simple documentation" of the JSON API and add it to an advanced section.

@leo-da
Copy link
Contributor

leo-da commented Dec 12, 2019

How would the client-side know which packages are there and which aren't? ...

you will receive a JSON output with an error that contains a variant describing that exact error scenario, and this variant will contain a list of unknown templateIds as they were passed from the client.

Originally I thought we are talking about a search which is a part of some automated flow. From the above I understand you are referring to a manual search.

So, what is wrong with asking a user to fix the search criteria because user specified an unknown template ID. As a user I would personally prefer to be notified that the template ID I am looking for does not exist any longer. Or just to know that there is a typo in one of my template IDs: lout:lou_Transfer, Iou:IouTrаnsfer... go figure if used any Cyrillics in the above template ID. The user will keep searching for a contract and will keep receiving empty search result back.

In any case, if that is the behavior you want, talk with @bame-da and @da-tanabe, come up with something that makes everyone happy, document the requirements and prioritize this issue. I will be happy to work on it.

@leo-da
Copy link
Contributor

leo-da commented Dec 12, 2019

What would be the downside of making the behavior in case of non-existing template ids configurable? We can leave this out of the "simple documentation" of the JSON API and add it to an advanced section.

the downside is complexity, we will have to support two modes and two sets of integration tests.

@leo-da
Copy link
Contributor

leo-da commented Dec 12, 2019

How about JSON API returns HTTP 400, but with both results and errors for every unknown template ID. So you will have the data that JSON API was able to retrieve + error that tells user that they were searching for something that does not exist?

{
"status": 400,
"result": [....],
"errors": [....]
}

It will be up to the caller to decide what to do with the result and errors. If they want they can:

  • display only result
  • display only errors and ask user to fix the input
  • display both

@da-tanabe
Copy link
Contributor

What @hurryabit has said is exactly what I would have said as well. In principle I should be able to write a web app and have no errors if I did everything correctly. My mental model is firmly in the camp of @bame-da's (2).

@S11001001
Copy link
Contributor

I don't think we should have any switches. @leo-da 's always including result and errors proposal, and always including nonexistent template IDs as errors, makes sense to me, and satisfies either style.

@hurryabit
Copy link
Contributor

I'm fine with always returning a list of template ids that could not be resolved. I suggest we use 200 as the HTTP status code if unresolved template ids are the only "error" (I rather consider them to be warnings). Otherwise, our users' users will start seeing errors in their browser console just because our users have cleaned up their ledgers.

@leo-da
Copy link
Contributor

leo-da commented Dec 12, 2019

Otherwise, our users' users will start seeing errors in their browser console just because our users have cleaned up their ledgers.

which is fine with me, you get an error, -- you refine your search criteria, else there will be no way for end users to get notified about the non existent template IDs (unless you try to submit a new contract for this non-existing template).

It is just strange that you will be able to search for contracts using non-existing template ID, get HTTP 200/success in response... but if you try to submit a new contract with the same template ID you get an error saying you can't use this template ID any more!

@leo-da
Copy link
Contributor

leo-da commented Dec 12, 2019

again, it is up to the person who implements the UI to handle the proposed use case when you get HTTP 400 with both results and errors, if you think it is not an error, just don't display it, display only the results which may be empty if the only thing you specified is a non-existing template ID.

If you are concerned about errors/warnings in the browser console, just do not log them. I don't think it is a default behavior to log 400.

@hurryabit
Copy link
Contributor

I would like to come to a conclusion about this ticket. Can we please implement the following behavior when you query for templates that don't exist:

  1. You get HTTP status code 200.
  2. The response body contains a field named unknownTemplateIds or similar that mentions alls template ids that do not exist on the ledger.

@leo-da
Copy link
Contributor

leo-da commented Jan 7, 2020

  1. OK, if you insist.
  2. unknownTemplateIds will be in the errors, don't know exactly how the response will look but probably something like this:
{
  "status": 200,
  "result": [{...}, {...}...],
  "errors": [
    {"code": 101, "unknownTemplateIds": ["AAA:BBB:CCC", "DDD:EEE:FFF"]}
  ]
}

@hurryabit
Copy link
Contributor

What is code 101?

@leo-da
Copy link
Contributor

leo-da commented Jan 8, 2020

What is code 101?

custom error code/type guard. So you can match on it from TypeScript.

Your decodeLedgerError will have to change, it currently expects an array of string errors.

const decodeLedgerError: jtv.Decoder<LedgerError> = jtv.object({
  status: jtv.number(),
  errors: jtv.array(jtv.string()),
});

Is that fine?... or do you really want optional unknownTemplateIds:

{
  "status": 200,
  "result": [{...}, {...}...],
  "unknownTemplateIds": ["AAA:BBB:CCC", "DDD:EEE:FFF"]}
}

@hurryabit
Copy link
Contributor

I find this error code thing quite ad hoc. As I said before, I also regard it more as a warning and would prefer to not mix it up with actual errors. I'd very much prefer the optional unknownTemplateIds you showed above.

@hurryabit
Copy link
Contributor

hurryabit commented Jan 9, 2020

{
  "status": 200,
  "result": [{...}, {...}, ...],
  "warnings": {
    "unknownTemplateIds": ["AAA:BBB:CCC", "DDD:EEE:FFF"]
  }
}

@leo-da leo-da added the wip-issue This issue is being worked on. Use draft PRs for work in progress PRs. label Jan 9, 2020
leo-da added a commit that referenced this issue Jan 10, 2020
[JSON API - Experimental]
``/contracts/search`` endpoint reports unresolved template IDs as warnings, see #3771::

    {
        "warnings": {
            "unknownTemplateIds": ["UnknownModule:UnknownEntity"]
        },
        "result": [{...}, ...],
        "status": 200
    }

CHANGELOG_END
leo-da added a commit that referenced this issue Jan 10, 2020
[JSON API - Experimental]
``/contracts/search`` endpoint reports unresolved template IDs as warnings, see #3771::

    {
        "warnings": {
            "unknownTemplateIds": ["UnknownModule:UnknownEntity"]
        },
        "result": [{...}, ...],
        "status": 200
    }

CHANGELOG_END
leo-da added a commit that referenced this issue Jan 14, 2020
[JSON API - Experimental]
``/contracts/search`` endpoint reports unresolved template IDs as warnings, see #3771::

    {
        "warnings": {
            "unknownTemplateIds": ["UnknownModule:UnknownEntity"]
        },
        "result": [{...}, ...],
        "status": 200
    }

CHANGELOG_END
@mergify mergify bot closed this as completed in #4014 Jan 14, 2020
mergify bot pushed a commit that referenced this issue Jan 14, 2020
…late IDs in the `warnings` element (#4014)

* WIP

* test cases updated

* `PackageService.resolveTemplateId` returns `Option[TemplateId]` now

used to return `Error \/ TemplateId`. There are scenarios when unresolved
TemplateID is a warning and not an error, which was the initial design

* CHANGELOG_BEGIN

[JSON API - Experimental]
``/contracts/search`` endpoint reports unresolved template IDs as warnings, see #3771::

    {
        "warnings": {
            "unknownTemplateIds": ["UnknownModule:UnknownEntity"]
        },
        "result": [{...}, ...],
        "status": 200
    }

CHANGELOG_END

* Addressing codereview comments

* `partitionMap` is now part of `http.util.Collections.SeqOps`
@stefanobaghino-da stefanobaghino-da added component/json-api HTTP JSON API team/ledger-clients Related to the Ledger Clients team's components. labels Aug 31, 2021
@S11001001 S11001001 removed the wip-issue This issue is being worked on. Use draft PRs for work in progress PRs. label Nov 2, 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
7 participants