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

[AERIE-1691] Return default arguments alongside effective argument failures #50

Merged
merged 3 commits into from
Feb 18, 2022

Conversation

pcrosemurgy
Copy link
Contributor

@pcrosemurgy pcrosemurgy commented Feb 16, 2022

  • Tickets addressed: AERIE-1691
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

Adds default arguments alongside the getModelEffectiveArguments /getActivityEffectiveArguments failure response. This will allow the UI to implement default values within forms with the getModelEffectiveArguments /getActivityEffectiveArguments workflow.
See AERIE-1691 for more context.

Verification

Expand

Success Responses

With examples/banananation's all optional arguments:

query TestGetModelEffectiveArguments {
  getModelEffectiveArguments(missionModelId: 1, modelArguments: {}) {
    success
    errors
    arguments
  }
}

{
  "data": {
    "getModelEffectiveArguments": {
      "success": true,
      "arguments": {
        "initialDataPath": "/etc/os-release",
        "initialPlantCount": 200,
        "initialProducer": "Chiquita"
      }
    }
  }
}

Failure Responses

With BakeBananaBread, where only the temperature parameter has a default value:

query TestGetActivityEffectiveArguments {
  getActivityEffectiveArguments(missionModelId: 1, activityTypeName: "BakeBananaBread", activityArguments: {}) {
    success
    errors
    arguments
  }
}

{
  "data": {
    "getActivityEffectiveArguments": {
      "success": false,
      "errors": {
        "tbSugar": {
          "schema": {
            "type": "int"
          },
          "message": "Required argument for activity \"BakeBananaBread\" not provided: \"tbSugar\" of type ValueSchema.INT"
        },
        "glutenFree": {
          "schema": {
            "type": "boolean"
          },
          "message": "Required argument for activity \"BakeBananaBread\" not provided: \"glutenFree\" of type ValueSchema.BOOLEAN"
        }
      },
      "arguments": {
        "temperature": 350
      }
    }
  }
}

With examples/config-without-defaults's all required arguments:

query TestGetModelEffectiveArguments {
  getModelEffectiveArguments(missionModelId: 1, modelArguments: {}) {
    success
    errors
    arguments
  }
}

{
  "data": {
    "getModelEffectiveArguments": {
      "success": false,
      "errors": {
        "a": {
          "schema": {
            "type": "int"
          },
          "message": "Required argument for configuration \"Configuration\" not provided: \"a\" of type ValueSchema.INT"
        },
        "b": {
          "schema": {
            "type": "real"
          },
          "message": "Required argument for configuration \"Configuration\" not provided: \"b\" of type ValueSchema.REAL"
        },
        "c": {
          "schema": {
            "type": "string"
          },
          "message": "Required argument for configuration \"Configuration\" not provided: \"c\" of type ValueSchema.STRING"
        }
      },
      "arguments": {}
    }
  }
}

Documentation

Updated Aerie GQL API documentation: https://github.com/NASA-AMMOS/aerie/wiki/Aerie-GraphQL-API-Software-Interface-Specification#query-for-activity-effective-arguments

Future work

None.

@pcrosemurgy pcrosemurgy changed the title Feature/aerie 1691 effective args defaults [AERIE-1691] Return default arguments alongside effective argument failures Feb 16, 2022
@pcrosemurgy pcrosemurgy force-pushed the feature/aerie-1691-effective-args-defaults branch 5 times, most recently from 68db1b3 to 0c612ca Compare February 16, 2022 23:25
@pcrosemurgy pcrosemurgy marked this pull request as ready for review February 16, 2022 23:32
Copy link
Member

@camargo camargo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally and implemented a change on top of it in the UI. LGTM!

Copy link
Contributor

@kroffo kroffo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall. One minor issue to fix, and one comment to address, but nothing showstopping, so approved!

Copy link
Collaborator

@mattdailis mattdailis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I was a bit skeptical of the exception with a builder pattern, but then I looked at the generated code, and it looked quite elegant. Good job.

@pcrosemurgy pcrosemurgy force-pushed the feature/aerie-1691-effective-args-defaults branch from 0c612ca to 0592816 Compare February 18, 2022 18:00
@pcrosemurgy pcrosemurgy force-pushed the feature/aerie-1691-effective-args-defaults branch from 0592816 to 8bf23d8 Compare February 18, 2022 18:22
@pcrosemurgy
Copy link
Contributor Author

I was a bit skeptical of the exception with a builder pattern, but then I looked at the generated code, and it looked quite elegant.

Good point, and I think it should get some skepticism.

I'm not too fond of using an exception as a required part of our UI <-> mission model workflow. The only alternative I could think of was to split getEffectiveArguments into 2 methods – and then complicate the already complex UI argument cobbling logic by requiring yet another endpoint to be hit.

I guess the argument for the pattern this PR uses is that we're not developing the mission model interface with UI concerns as the 1st priority, and that'll sometimes result in the UI relying on an exception to get ancillary data (such as returning default arguments when required args. are missing).

@pcrosemurgy pcrosemurgy merged commit dbe4c33 into develop Feb 18, 2022
@pcrosemurgy pcrosemurgy deleted the feature/aerie-1691-effective-args-defaults branch February 18, 2022 18:30
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 this pull request may close these issues.

4 participants