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.

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

add json parser #140

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

add json parser #140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
981b15d
add json parser
dschadd Apr 24, 2023
b950040
include spec
dschadd Apr 24, 2023
b111d4f
add spec and force json parse on everything
dschadd Apr 25, 2023
1600632
change version
dschadd Apr 25, 2023
2be0b96
update version in gemfile
dschadd Apr 25, 2023
e0e564f
fix version
dschadd Apr 25, 2023
21d8eb4
actually fix version
dschadd Apr 25, 2023
11b9209
update readme
dschadd Apr 25, 2023
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
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ Decanter comes with the following parsers out of the box:
- `:phone`
- `:string`
- `:array`
- `:json`

Note: these parsers are designed to operate on a single value, except for `:array`. This parser expects an array, and will use the `parse_each` option to call a given parser on each of its elements:

Expand Down
12 changes: 12 additions & 0 deletions lib/decanter/parser/json_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module Decanter
module Parser
class JsonParser < ValueParser
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this should inherit from HashParser

See https://github.com/LaunchPadLab/decanter#custom-parser-base-classes

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that doesn't account for the fact that it could be an array. NVM!

Copy link
Author

Choose a reason for hiding this comment

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

Yea it turns out that JSON is anything lol


Copy link
Author

Choose a reason for hiding this comment

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

Since json is not a native Ruby data type, there is no specific allowed types. Instead all values passed to this parser will be parsed by JSON.parse(val), unless they are nil or blank strings.

parser do |val, options|
raise Decanter::ParseError.new 'Expects a JSON string' if val.is_a?(Array) || val.is_a?(Hash)
Copy link
Author

Choose a reason for hiding this comment

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

JSON.parse only accepts string type as an argument

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgive my ignorance here – does ActiveRecord require that the input to create/update is a hash? Or will it accept a serialized JSON string?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of checking for an Array or a Hash as opposed to is_a? String?

Copy link
Author

Choose a reason for hiding this comment

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

  1. I'm having a hard time finding exact documentation, but I don't think there is a specific requirement for it to be hash. Mainly because JSON can be an array, hash, string

  2. I think I had it this way just for readability as to avoid unless, but looking at it again I'm not sure that's any more readable. I think I'll change it to unless is_a? String

This line in particular is difficult since JSON can be many things. I almost decided to remove this entirely, but wanted to keep some error message here. I could make an argument for having no error message here, but that felt wrong?

I leaned into the fact the client will send it as a string, I will check that, and then the code on line 8 will parse it to become a JSON object for Rails to use

Copy link
Contributor

Choose a reason for hiding this comment

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

From some quick testing in the console, it looks like JSON.parse throws an error for anything that's not a string (including booleans, integers, etc).

Because of that, to me the most readable option seems like unless is_a? String, but I also wonder if a readable alternative could be rescuing from a JSON.parse error to tell the dev that the value is not valid JSON.

next if (val.nil? || val === '')
JSON.parse(val)
end
end
end
end
26 changes: 26 additions & 0 deletions spec/decanter/parser/json_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'spec_helper'

describe 'JsonParser' do

let(:name) { :foo }

let(:parser) { Decanter::Parser::JsonParser }

describe '#parse' do
it 'parses string value and returns a parsed JSON' do
expect(parser.parse(name, '{"key": "value"}')).to match({name => {"key" => "value"}})
end

context 'with empty string' do
it 'returns nil' do
expect(parser.parse(name, '')).to match({name => nil})
end
end

context 'with nil' do
it 'returns nil' do
expect(parser.parse(name, nil)).to match({name => nil})
end
end
end
end