disruptek / atoz

Amazon Web Services (AWS) APIs in Nim
MIT License
48 stars 5 forks source link

Dynamodb 20120810 call scan body is empty #7

Open hugosenari opened 2 years ago

hugosenari commented 2 years ago

Hi, again.

I found a possible bug in Dynamodb 20120810 scan. When I call it, body of Recallable is empty.

  let
    path: JsonNode = nil
    query: JsonNode = nil
    header: JsonNode = nil
    formData: JsonNode = nil
    body = %* {"TableName": "ReSSt"}
    request = scan.call(path, query, header, formData, body)
    response = waitfor request.issueRequest()
  echo "payload ", request.body # is empty
  echo "status ", response.status # error 400 invalid request

The problems is that atozHook is expecting a jString but scan.call expects body as jObject

disruptek commented 2 years ago

I run into this frequently. The main cause is a change I made a long time ago to appease one user or another; I didn't really think it through with respect to atoz and the behavior of AWS APIs varies enough that it causes problems in practice.

I have a 1000 line dynamo API built on atoz and it's full of garbage like this:

  import dynamodb_20120810 as ddb
  # imagine this getItem call;
  # let's call `js` our JObject holding the query...
  js["TableName"] = newJString table.name
  js["Key"] = key.toDynamo
  js["ConsistentRead"] = newJBool true
  # ...
  ddb.getItem.call(path = nil, _ = $js)

It's not pretty, but what's tricky about this stuff is doing the right thing across all over the AWS APIs without changing the openapi library to the point where it doesn't work correctly.

Or it's possible that I'm completely wrong about what the problem is. But, try this approach first and let me know. :wink:

hugosenari commented 2 years ago

I tried similar approach, but it behaves different from what I would expect (return 404) :shrug:

request = scan.call(_ = $body)

Have you ever considered trying 'aws way' (use some proprietary json instead of OpenApi)?

hugosenari commented 2 years ago

I changed my local copy of atoz

method atozHook(call: OpenApiRestCall; url: Uri; input: JsonNode; body = ""): Recallable {.
    base.} =
  ## the hook is a terrible earworm
  var
    headers = newHttpHeaders(massageHeaders(input.getOrDefault("header")))
    text = body
  if text.len == 0 and "body" in input:
-    text = input.getOrDefault("body").getStr
+    text = $input.getOrDefault("body")
    if not headers.hasKey("content-type"):
      headers["content-type"] = "application/x-amz-json-1.0"
  else:
    headers["content-md5"] = base64.encode text.toMD5
  if not headers.hasKey($SecurityToken):
    let session = getEnv("AWS_SESSION_TOKEN", "")
    if session != "":
      headers[$SecurityToken] = session
  result = newRecallable(call, url, headers, text)
  result.atozSign(input.getOrDefault("query"), SHA256)

It worked, I'm not sure why we should expect body to be a jString with body instead of be JsonNode of body :thinking:

Maybe

-    text = input.getOrDefault("body").getStr
+    if input.getOrDefault("body").kind == JString:
+      text = input.getOrDefault("body").getStr
+    else:
+      text = $input.getOrDefault("body")
disruptek commented 2 years ago

The problem is

assert $(JString"foo") != getStr(JString"foo") # foo versus "foo"

We have some code that does this same dance elsewhere; if we receive a JString we'll just render the string value, else we'll render true JSON. It kinda sucks and the proper solution is probably to use the hooks in openapi to fiddle with the values during validation or something.

I'm looking at the generator and what I'm using for my codegen at work is slightly different, so maybe it's time for me to just put the generator into the repo and remove the artifacts. It's already a problem supporting git-lfs for some reason.

And, FWIW, I don't mind replacing atoz with something else, but I don't use Nim anymore, so motivation is low. However, I don't mind scheming on how to go about it, so to speak, because it looks like I need to build a new atoz for Racket, so... :wink:

disruptek commented 2 years ago

Okay, this https://github.com/disruptek/atoz/releases/tag/2626.4.0 has the generator in it so you can mess around more easily.

hugosenari commented 2 years ago

Sorry I didn't have time to play with it yet, I'll just leave my thoughts here to not forget or help someone else (maybe you with Racket implementation).

assert $(JString"foo") != getStr(JString"foo") # foo versus "foo"

You're correct, I was thinking about the symptoms, not the problems.

General rule:

  1. if OpenAPI file say application/json it should be expecting $(JString"foo")
  2. else if OpenAPI file say text/plain for getStr(JString"foo")

Possible issues:

  1. Code isn't respecting content-type of OpenAPI file, is a generator issue.
  2. If OpenAPI file say application/json but server expects text/plain, bug is in OpenAPI file.
  3. if OpenAPI file say application/json and server expects application/json but with getStr(JString"foo"), is a server issue

We don't own OpenAPI file, but with appropriate information, I could register an issue there.

disruptek commented 2 years ago

I doubt my openapi code respects/sniffs (or even surfaces) the MIME type, so that's clearly a problem.

Basically, I hated Nim's httpclient so much that I didn't want to write the whole request/response loop using it; I assumed it would be replaced Soon so I delayed that effort. The predictable result, of course, was that I ended up using it for years with a hodge-podge of lame hacks. :facepalm:

As far as this problem goes, my openapi stuff was written against OpenAPI 2.x and while there's a PR to upgrade it to support OpenAPI 3.x, that work seems to've stalled.

The AWS APIs exported at https://github.com/APIs-guru/openapi-directory/tree/main/APIs/amazonaws.com don't appear to've been updated in awhile, but in any event, they are now OpenAPI 3-based.

hugosenari commented 2 years ago

Random information for different approach: AWS seems to be working on Smithy, that can read 'AWS API definitions' (JSON) and Smithy DSL for API definitions.

I couldn't find any .smithy definitions of AWS API, only JSON :shrug: if APIs-guru decease, Smithy has a tool for OpenAPI generation There is an AWS SDK for rust with smithy, but is written in Kotlin

disruptek commented 2 years ago

Well, I know a few languages are doing this automagically using the Go API assets, including Clojure and Elixir; https://github.com/aws-beam/aws-elixir recently started automatically pushing codegen into trunk.

I thought Smithy was just what they were going to use in API Gateway but obviously that would be preferable for semantic reasons, if the AWS APIs are actually published in Smithy, I mean.