Conversation
choubacha
left a comment
choubacha
left a comment
There was a problem hiding this comment.
This is a really great feature and tool! :)
I think before merging we should add some specs around the parsing of the query itself. That way we have examples that demonstrate and validate common inputs.
| class Helpers | ||
| class << self | ||
| def parse_query(query_string) | ||
| has_open_brace = query_string.include?("{") |
There was a problem hiding this comment.
I know it's generally a flat JSON, but perhaps we could use a regex here to anchor the opening and closing brace to the start and end? I think we could also strip the string and then look at the first and last character.
| # first let's see if it parses | ||
| begin | ||
| query_attributes = JSON.parse(query_string) | ||
| raise "Not a JSON Object" unless query_attributes.is_a?(Hash) |
There was a problem hiding this comment.
I think we could also just set the exception variable here. That would avoid the raise and immediate rescue.
lib/resque_bus/server.rb
Outdated
| begin | ||
| query_attributes = JSON.parse(query_string) | ||
| raise "Not a JSON Object" unless query_attributes.is_a?(Hash) | ||
| rescue Exception => e |
There was a problem hiding this comment.
We probably shouldn't rescue Exception as that include out of memory errors and other system errors. We should either target StandardError by omitting it, or rescue JSON parse errors specifically.
lib/resque_bus/server.rb
Outdated
| if query_attributes | ||
| # it parsed but it's something else | ||
| if query_attributes.is_a?(Array) && query_attributes.length == 1 | ||
| # maybe it's the thing from the queue |
There was a problem hiding this comment.
Could you expand this comment to explain what "the thing from the queue" is? It sounds like a horror movie.
| # maybe it's the thing from the queue | ||
| json_string = query_attributes.first | ||
| fixed = JSON.parse(json_string) rescue nil | ||
| return fixed if fixed |
There was a problem hiding this comment.
Should we validate that this is also a Hash as we do earlier?
|
|
||
| if !has_open_brace && !has_close_brace | ||
| # maybe they just forgot the braces | ||
| fixed = JSON.parse("{ #{query_string} }") rescue nil |
There was a problem hiding this comment.
Couldn't we do this on line 38 before we parse? If we're concerned about the possibility that it's an array, maybe we can also look at the opening/closing brackets to see if they are square braces.
There was a problem hiding this comment.
One thing I tried to do was not mess with the input if it would parse. That's why I parsed first. Seems like a safe practice - if they put something that works use it before messing around with it.
| return fixed if fixed | ||
| end | ||
|
|
||
| if !has_open_brace |
There was a problem hiding this comment.
Might be able to reduce the complexity. of these conditions:
query_string = "{ #{query_string}" unless has_open_brace
query_string = "#{query_string} }" unless has_close_brace
fixed = JSON.parse(query_string) rescue nil
return fixed if fixed| if query_string.length > 0 | ||
| begin | ||
| query_attributes = ::ResqueBus::Server::Helpers.parse_query(query_string) | ||
| raise "Not a JSON Object" unless query_attributes.is_a?(Hash) |
There was a problem hiding this comment.
I think it would make the view lighter if we put this logic into the parser. This is already repeated with some of the logic in the parse_query fuction.
|
|
||
| class Helpers | ||
| class << self | ||
| def parse_query(query_string) |
There was a problem hiding this comment.
Could this move to QueueBus so that it can be shared between sidekiq-bus and resque-bus?
There was a problem hiding this comment.
sounds like a plan to me.
|
added some specs. |
2 participants
Uh oh!
There was an error while loading. Please reload this page.