Skip to content

bugfix: Handle media type json a bit friendlier #1876

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.

Sign up for GitHub

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

Open
luke-hill wants to merge 11 commits into
base: main
Choose a base branch
from
Open

bugfix: Handle media type json a bit friendlier #1876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b131235
Handle media type json a bit friendlier
luke-hill May 22, 2026
ae22a8a
Update message_builder.rb
luke-hill May 22, 2026
9ea31c3
Fix invalid key
luke-hill May 22, 2026
b468936
Invert the logic and instead only stream readable entities otherwise …
luke-hill May 22, 2026
d5986d1
Update changelog
luke-hill May 22, 2026
21bbcae
Fix method signature
luke-hill May 22, 2026
139f9da
Fix attach and streamed file logic for formatters plus message output
luke-hill May 22, 2026
af3f0fb
Fix step def specs
luke-hill May 22, 2026
797e043
Fix console formatting
luke-hill May 22, 2026
baa670d
Fix json formatter test
luke-hill May 22, 2026
68f0cb0
Final tidy for defaults
luke-hill May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ Please visit [cucumber/CONTRIBUTING.md](https://github.com/cucumber/cucumber/blo
### Changed
- Heavy refactor to the internals for message building (Used in formatters - should be no noticeable change)
([#1853](https://github.com/cucumber/cucumber-ruby/pull/1853) [luke-hill](https://github.com/luke-hill))
- Simplify attachment handling in the `MessageBuilder` and `#attach` method

([#1853](
### Fixed
- When someone `#attach`s a hashified output (Instead of JSON), call `#to_json` before attaching as a stringified JSON response

## [11.0.0] - 2026-04-14
### Added
Expand Down
Binary file added features/docs/fixtures/cucumber.jpeg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 9 additions & 12 deletions features/docs/writing_support_code/attachments.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,37 +20,34 @@ Feature: Attachments
Scenario:
Given I attach a screenshot without a media type
"""
And a file named "features/screenshot.png" with:
"""
foo
"""
And an image named "cucumber.jpeg"
And a file named "features/step_definitions/attaching_screenshot_steps.rb" with:
"""
Given('I attach a screenshot with a media type') do
attach('features/screenshot.png', 'image/png')
attach('features/cucumber.jpeg', 'image/jpeg')
end

Given('I attach a screenshot without a media type') do
attach('features/screenshot.png')
attach('features/cucumber.jpeg')
end
"""

Scenario: Files can be attached given their path
When I run `cucumber --format message features/attaching_screenshot_with_mediatype.feature`
Then output should be valid NDJSON
And the output should contain NDJSON with key "attachment"
And the output should contain NDJSON "attachment" message with key "body" and value "Zm9v"
And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/png"
And the output should contain NDJSON "attachment" message with key "body" and an encoded value
And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/jpeg"

Scenario: Media type is inferred from the given file
When I run `cucumber --format message features/attaching_screenshot_without_mediatype.feature`
Then output should be valid NDJSON
And the output should contain NDJSON with key "attachment"
And the output should contain NDJSON "attachment" message with key "body" and value "Zm9v"
And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/png"
And the output should contain NDJSON "attachment" message with key "body" and an encoded value
And the output should contain NDJSON "attachment" message with key "mediaType" and value "image/jpeg"

Scenario: With json formatter, files can be attached given their path
When I run `cucumber --format json features/attaching_screenshot_with_mediatype.feature`
Then the output should contain "embeddings\":"
And the output should contain "\"mime_type\": \"image/png\","
And the output should contain "\"data\": \"Zm9v\""
And the output should contain "\"mime_type\": \"image/jpeg\","
And the output should contain "\"data\":"
2 changes: 1 addition & 1 deletion features/lib/step_definitions/cucumber_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
' @io = config.out_stream',
' end',
'',
' def attach(src, media_type, _filename)',
' def attach(src, media_type, _filename, _streamed_file)',
' @io.puts(src)',
' end',
'end'
Expand Down
4 changes: 4 additions & 0 deletions features/lib/step_definitions/filesystem_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
write_file(path, content)
end

Given('an image named {string}') do |name|
copy_image_named(name)
end

Given('an empty file named {string}') do |path|
write_file(path, '')
end
Expand Down
6 changes: 6 additions & 0 deletions features/lib/step_definitions/message_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@
expect(command_line.stdout).to match(/"#{key}": ?"#{value}"/)
end

Then('the output should contain NDJSON {string} message with key {string} and an encoded value') do |message_name, key|
message_contents = command_line.stdout(format: :messages).detect { |msg| msg.keys == [message_name] }[message_name]

expect(message_contents[key].length).to be > 50
end

Then('the output should contain NDJSON {string} message with key {string} and value {string}') do |message_name, key, value|
message_contents = command_line.stdout(format: :messages).detect { |msg| msg.keys == [message_name] }[message_name]

Expand Down
5 changes: 5 additions & 0 deletions features/lib/support/filesystem.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,8 @@ def write_file(path, content)
FileUtils.mkdir_p(File.dirname(path))
File.open(path, 'w') { |file| file.write(content) }
end

def copy_image_named(name)
fixture_dir = File.expand_path('../../features/docs/fixtures')
FileUtils.cp("#{fixture_dir}/#{name}", "#{Dir.pwd}/features/#{name}")
end
2 changes: 1 addition & 1 deletion lib/cucumber/formatter/console.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def do_print_passing_wip(passed_messages)
end
end

def attach(src, media_type, filename)
def attach(src, media_type, filename, _streamed_file)
return unless media_type == 'text/x.cucumber.log+plain'
return unless @io

Expand Down
2 changes: 1 addition & 1 deletion lib/cucumber/formatter/json.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def on_test_run_finished(_event)
@io.write(JSON.pretty_generate(@feature_hashes))
end

def attach(src, mime_type, _filename)
def attach(src, mime_type, _filename, _streamed_file)
if mime_type == 'text/x.cucumber.log+plain'
test_step_output << src
return
Expand Down
16 changes: 8 additions & 8 deletions lib/cucumber/formatter/message_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
# frozen_string_literal: true

require 'base64'
require 'cucumber/formatter/backtrace_filter'
require 'json'

require 'cucumber/formatter/backtrace_filter'
require 'cucumber/query'

module Cucumber
Expand Down Expand Up @@ -55,7 +56,7 @@ def initialize(config)
config.on_event :undefined_parameter_type, &method(:on_undefined_parameter_type)
end

def attach(src, media_type, filename)
def attach(src, media_type, filename, streamed_file)
attachment_data = {
test_step_id: @current_test_step_id,
test_case_started_id: @current_test_case_started_id,
Expand All @@ -64,13 +65,12 @@ def attach(src, media_type, filename)
timestamp: time_to_timestamp(Time.now)
}

if media_type&.start_with?('text/')
attachment_data[:content_encoding] = Cucumber::Messages::AttachmentContentEncoding::IDENTITY
attachment_data[:body] = src
else
body = src.respond_to?(:read) ? src.read : src
if streamed_file
Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Currently the behaviour of the function doesn't seem to guarantee a valid message is produced in every scenario.

And asking the user of the API for this decision doesn't seem to be quite right either.

For example it look like it is possible to attach a byte stream with identity encoding by passing streamed_file = true.

Is there a way you can detect streamed files from their type or duck-type?

Then you can write:

if string
  attach as string with identity encoding
else if hash:
  attach with to_json with identity encoding
else if byte stream
  attach with strict_encode64 and base64 encoding.
else 
  complain about unsupported type.

attachment_data[:content_encoding] = Cucumber::Messages::AttachmentContentEncoding::BASE64
attachment_data[:body] = Base64.strict_encode64(body)
attachment_data[:body] = Base64.strict_encode64(src)
else
attachment_data[:content_encoding] = Cucumber::Messages::AttachmentContentEncoding::IDENTITY
attachment_data[:body] = src.is_a?(Hash) ? src.to_json : src
Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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".

Copy link
Copy Markdown
Contributor Author

@luke-hill luke-hill May 22, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 22, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@luke-hill luke-hill May 22, 2026

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@luke-hill luke-hill May 22, 2026

Choose a reason for hiding this comment

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

Or just thinking about it, we could invert this.

And write something like

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

Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@mpkorstanje mpkorstanje May 22, 2026

Choose a reason for hiding this comment

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

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. 😄

Copy link
Copy Markdown
Contributor Author

@luke-hill luke-hill May 22, 2026

Choose a reason for hiding this comment

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

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.

end

message = Cucumber::Messages::Envelope.new(attachment: Cucumber::Messages::Attachment.new(**attachment_data))
Expand Down
2 changes: 1 addition & 1 deletion lib/cucumber/formatter/pretty.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def on_test_run_finished(_event)
print_summary
end

def attach(src, media_type, filename)
def attach(src, media_type, filename, _streamed_file)
return unless media_type == 'text/x.cucumber.log+plain'

if filename
Expand Down
12 changes: 7 additions & 5 deletions lib/cucumber/glue/proto_world.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,16 @@ def log(*messages)
# @param filename [string] the name of the file you wish to specify.
# This is only needed in situations where you want to rename a PDF download e.t.c. - In most situations
# you should not need to pass a filename
def attach(file, media_type = nil, filename = nil)
def attach(file, media_type = nil, filename = nil, streamed_file = nil)
if File.file?(file)
media_type = MiniMime.lookup_by_filename(file)&.content_type if media_type.nil?
file = File.read(file, mode: 'rb')
streamed_file = true
end
super
# We pass in the concept of whether the file is streamed to ensure that the envelope encoding is correct
super(file, media_type, filename, streamed_file)
rescue StandardError
super
super(file, media_type, filename, streamed_file)
end

# Mark the matched step as pending.
Expand Down Expand Up @@ -153,8 +155,8 @@ def add_modules!(world_modules, namespaced_world_modules)
runtime.ask(question, timeout_seconds)
end

define_method(:attach) do |file, media_type, filename|
runtime.attach(file, media_type, filename)
define_method(:attach) do |file, media_type, filename, streamed_file = false|
runtime.attach(file, media_type, filename, streamed_file)
end

# Prints the list of modules that are included in the World
Expand Down
4 changes: 2 additions & 2 deletions lib/cucumber/runtime/user_interface.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ def ask(question, timeout_seconds)
# be a path to a file, or if it's an image it may also be a Base64 encoded image.
# The embedded data may or may not be ignored, depending on what kind of formatter(s) are active.
#
def attach(src, media_type, filename)
@visitor.attach(src, media_type, filename)
def attach(src, media_type, filename, streamed_file)
@visitor.attach(src, media_type, filename, streamed_file)
end

private
Expand Down
20 changes: 9 additions & 11 deletions spec/cucumber/glue/step_definition_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,15 +63,15 @@ def step_match(text)
it 'calls a method on the world when specified with a symbol' do
expect(registry.current_world).to receive(:with_symbol)

dsl.Given(/With symbol/, :with_symbol)
dsl.Given('With symbol', :with_symbol)

run_step 'With symbol'
end

it 'calls a method on a specified object' do
allow(registry.current_world).to receive(:target) { target }

dsl.Given(/With symbol on block/, :with_symbol, on: -> { target })
dsl.Given('With symbol on block', :with_symbol, on: -> { target })

expect(target).to receive(:with_symbol)

Expand All @@ -81,7 +81,7 @@ def step_match(text)
it 'calls a method on a specified world attribute' do
allow(registry.current_world).to receive(:target) { target }

dsl.Given(/With symbol on symbol/, :with_symbol, on: :target)
dsl.Given('With symbol on symbol', :with_symbol, on: :target)

expect(target).to receive(:with_symbol)

Expand All @@ -102,17 +102,15 @@ def step_match(text)
end

it 'raises UndefinedDynamicStep when an undefined step is parsed dynamically' do
dsl.Given(/Outside/) do
steps %(
Given Inside
)
dsl.Given('Outside') do
steps %(Given Inside)
end

expect { run_step 'Outside' }.to raise_error(Cucumber::UndefinedDynamicStep)
end

it 'raises UndefinedDynamicStep when an undefined step with doc string is parsed dynamically' do
dsl.Given(/Outside/) do
dsl.Given('Outside') do
steps %(
Given Inside
"""
Expand All @@ -125,7 +123,7 @@ def step_match(text)
end

it 'raises UndefinedDynamicStep when an undefined step with data table is parsed dynamically' do
dsl.Given(/Outside/) do
dsl.Given('Outside') do
steps %(
Given Inside
| a |
Expand Down Expand Up @@ -193,14 +191,14 @@ def step_match(text)

describe '#log' do
it 'calls "attach" with the correct media type' do
expect(user_interface).to receive(:attach).with('wasup', 'text/x.cucumber.log+plain', nil)
expect(user_interface).to receive(:attach).with('wasup', 'text/x.cucumber.log+plain', nil, nil)

dsl.Given('Loud') { log 'wasup' }
run_step 'Loud'
end

it 'calls `to_s` if the message is not a String' do
expect(user_interface).to receive(:attach).with('["Not", 1, "string"]', 'text/x.cucumber.log+plain', nil)
expect(user_interface).to receive(:attach).with('["Not", 1, "string"]', 'text/x.cucumber.log+plain', nil, nil)

dsl.Given('Loud') { log ['Not', 1, 'string'] }
run_step 'Loud'
Expand Down
Loading