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

Clarify the serialized wire format for complex error parameters #1217

Open
ash211 opened this issue Jul 18, 2022 · 2 comments · May be fixed by #1218
Open

Clarify the serialized wire format for complex error parameters #1217

ash211 opened this issue Jul 18, 2022 · 2 comments · May be fixed by #1218

Comments

@ash211
Copy link
Contributor

ash211 commented Jul 18, 2022

BLUF: we don't specify the wire format for complex error parameters, and conjure-java and conjure-typescript are behaving differently. We need to determine correct wire format here, and then update the language implementations to match.


Here is the current wire spec for error parameters: https://github.com/palantir/conjure/blob/master/docs/spec/wire.md#55-conjure-errors

parameters - a JSON map providing additional information regarding the nature of the error.

Unfortunately, the serialization format for these parameters is under-specified, and there's currently a mismatch between what conjure-java generates, and what conjure-typescript expects. Conjure-java effectively casts complex types to strings and sends the data as a string, whereas conjure-typescript generates .d.ts types expecting the complex types. Instead, it gets strings. This manifests as broken typescript code (at runtime!), and a difficulty accessing complex error parameters from typescript.

Example

Given this yml error definition:

    errors:
      NoAgentsAvailable:
        namespace: AgentAllocation
        code: CUSTOM_CLIENT
        safe-args:
          unhealthySha256: set<string>
          disabledSha256s: map<string, agentInfoService.AgentDisabledInfo>
          nonExistingSha256s: set<string>
        unsafe-args:
          unhealthy: set<AgentId>
          disabled: map<AgentId, agentInfoService.AgentDisabledInfo>
          nonExisting: set<AgentId>
          message: string

The conjure-java library generates errors that look like this (from chrome network tab):

{
    "errorCode": "CUSTOM_CLIENT",
    "errorName": "AgentAllocation:NoAgentsAvailable",
    "errorInstanceId": "682c79a4-b168-4971-acfe-0e58875ad890",
    "parameters": {
        "unhealthySha256": "[]",
        "disabledSha256s": "{}",
        "nonExistingSha256s": "[]",
        "unhealthy": "[]",
        "disabled": "{}",
        "nonExisting": "[]",
        "message": "No healthy and enabled agents available.  Check the status of assigned agents to ensure liveness."
    }
}

Note that the sha256s are strings, and not sets/maps.

The conjure-generated typescript definition expects these to be sets/maps, and not strings:

export interface INoAgentsAvailable {
    'errorCode': "CUSTOM_CLIENT";
    'errorInstanceId': string;
    'errorName': "AgentAllocation:NoAgentsAvailable";
    'parameters': {
        unhealthySha256: Array<string>;
        disabledSha256s: {
            [key: string]: IAgentDisabledInfo;
        };
        nonExistingSha256s: Array<string>;
        unhealthy: Array<string>;
        disabled: {
            [key: string]: IAgentDisabledInfo;
        };
        nonExisting: Array<string>;
        message: string;
    };
}

To handle this, currently FE typescript code is handling both types, outside of the conjure generator.

Sample:

// Conjure types lists of string in errors as string[], but they actually look like this:
// "[ri.foundry.main.dataset.1, ri.foundry.main.dataset.2]"
// So check if they're a string (for forwards-compatibility if it gets fixed) and then parse
// <snip internal issue link...>
export function conjureErrorRidsToRidsList(errorRids: string | string[]) {
    if (Array.isArray(errorRids)) {
        return errorRids;
    }
    // The array isn't valid JSON, we need to strip the square brackets and parse by hand
    const withoutBrackets = errorRids.substring(1, errorRids.length - 1);
    if (withoutBrackets.length === 0) {
        return [];
    }
    try {
        return withoutBrackets.split(", ");
    } catch (e) {
        console.error("conjureErrorRidsToRidsList: Unable to parse rids", e);
        return [];
    }
}
@pkoenig10
Copy link
Member

This has been a long standing issue with lots of prior discussion, see palantir/conjure-java#1812.

@ash211
Copy link
Contributor Author

ash211 commented Jul 19, 2022

Looks like we previously thought the wire spec does specify JSON serialization: palantir/conjure-java#1812 (comment) I'll add a clarifying example to the wire format doc.

ash211 added a commit that referenced this issue Jul 19, 2022
@ash211 ash211 linked a pull request Jul 19, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants