bugfix: Handle media type json a bit friendlier

[luke-hill]

Description

Partially fixes #1787

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds new behaviour)

Please add an entry to the relevant section of CHANGELOG.md as part of this pull request.

Checklist:

Your PR is ready for review once the following checklist is
complete. You can also add some checks if you want to.

• Tests have been added for any changes to behaviour of the code
• New and existing tests are passing locally and on CI
• bundle exec rubocop reports no offenses
• RDoc comments have been updated
• CHANGELOG.md has been updated

[luke-hill]

Hi @mitchgrout can you try this branch/PR and see if it fixes your problem

[luke-hill]

It's worth pointing out this actually changes the behaviour of attach. Which previously attached application/json bodies as encoded, now it attaches them unencoded. Which according to the CCK is the desired behaviour

[mpkorstanje]

Looking at the surrounding code, I don't think this implementation is correct.

The attachment encoding is currently derived from the media_type but should be derived from the type of src. If src is a string or equivalent type that can be presented properly as a json string then IDENTITY can be used. Otherwise BASE64 should be used.

[mpkorstanje]

In pseudo code:

if src instance of string-like:
          attachment_data[:content_encoding] = IDENTITY
          attachment_data[:body] = src
else if src instance of byte-array-or-stream-like
          attachment_data[:content_encoding] = BASE64
          attachment_data[:body] = Base64.strict_encode64(body)
else 
          throw "unsupported src type, must be string-like or byte-stream like".

[luke-hill]

I looked into the CCK for this. And for the CCK it wants to present JSON bodies as un-encoded.

cf: https://github.com/cucumber/compatibility-kit/blob/main/devkit/samples/attachments/attachments.ndjson#L46

[mpkorstanje]

The determination of encoding isn't attached to the media type but rather the type of the source.

This is matters because creating the the BASE64 encoding is rather expensive and difficult to read. So we only use it when the source can't be represented as a regular string.

You can see this in the step definitions. For all IDENTITY encodings we use either:

the string {string} is attached as {string}
the following string is attached as {string}:

https://github.com/cucumber/compatibility-kit/blob/b96ee7298b719b52cc8d9d0be3aa95b7dac20109/devkit/samples/attachments/attachments.ts#L5
https://github.com/cucumber/compatibility-kit/blob/b96ee7298b719b52cc8d9d0be3aa95b7dac20109/devkit/samples/attachments/attachments.ts#L22

While for the BASE64 we have:

an array with {int} bytes is attached as {string}

https://github.com/cucumber/compatibility-kit/blob/b96ee7298b719b52cc8d9d0be3aa95b7dac20109/devkit/samples/attachments/attachments.ts#L29

So I could do

String json = '{ "hello": "world" }';
scenario.attach(json, 'application/json') 

byte[] jsonBytes = json.toByteArray();
scenario.attach(jsonBytes, 'application/json') 

The first attachment would have IDENTITY while the second one would have BASE64. And we should probably make that explicit in a test somewhere.

[luke-hill]

This feels like a breakout I need to do then. Because this was just a patch to ensure that someone passing in a hash can represent it as a nice setup (Which it currently doesn't do anywhere).

[luke-hill]

Or just thinking about it, we could invert this.

And write something like

  if src.respond_to?(:read)
    # base64 logic
  else
    # identity logic
  end

[mpkorstanje]

If you look at the media type you'll be patching holes forever. For example what if you attach a hash with text/plain?

I do think the quick fix would be to only support string, src.respond_to?(:read) and whatever Base64.strict_encode64 accepts. Then throw an exception with an explanatory message for everything else.

But I don't know enough Ruby to help you here.

[mpkorstanje]

Or just thinking about it, we could invert this.

I don't know what :read does. It looks like duck typing, but I don't know the what it signifies. 😄

[luke-hill]

I dug into this code, it's untouched in about 7+ years. I think I'll do the bit I thought of just now. I'll fix it in here so it's done once and for all.