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.

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

Fix bug in tokenizer with nil source value #1873

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Fix bug in tokenizer with nil source value #1873

merged 1 commit into from
Dec 17, 2024

Conversation

bahar-p
Copy link
Member

@bahar-p bahar-p commented Dec 17, 2024

Resolves https://github.com/Shopify/shopify/issues/560829

Fixes the Tokenizer to handle source with null value gracefully. The longer explanation can be found in this comment as I was investigating a Bugsnag exception.

Tl;Dr
The Tokenizer isn't currently resilient to source with nil value. Liquid C does a graceful conversion here. This has been causing a lot of exception when we run liquid ruby and the asset value is nil. I followed the same pattern as Liquid C tokenizer to convert nil to empty string.

Test

I added a test and able to reproduce the exception with the test (before adding the fix).

Before

The new test fails with an exception. Tokenizer.new(nil).tokenize raises:

NoMethodError: undefined method `empty?' for nil
    lib/liquid/tokenizer.rb:31:in `tokenize'
    lib/liquid/tokenizer.rb:12:in `initialize'
    .....

After

The new test passes. Tokenizer.new(nil).tokenize returns [].

@bahar-p bahar-p changed the title Fix bug in tokenizer with nil value Fix bug in tokenizer with nil source value Dec 17, 2024
@bahar-p bahar-p force-pushed the bp/tokenizer-fix branch 2 times, most recently from fc27128 to 7574406 Compare December 17, 2024 15:33
@@ -5,7 +5,7 @@ class Tokenizer
attr_reader :line_number, :for_liquid_tag

def initialize(source, line_numbers = false, line_number: nil, for_liquid_tag: false)
@source = source
@source = source.to_s
Copy link
Member Author

@bahar-p bahar-p Dec 17, 2024

Choose a reason for hiding this comment

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

I'm not entirely sure why liquid_c does .to_s.to_str. 🤷🏼‍♀️

to_s should be sufficient, but let me know if I'm missing anything here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there was a legit reason originally to use both to_s.to_str, see #1421 (comment)

Looks like @ggmichaelgo then removed the logic here #1775

Copy link
Member Author

@bahar-p bahar-p Dec 17, 2024

Choose a reason for hiding this comment

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

Yeah the first PR didn't introduce a bug though. In the second PR @ggmichaelgo is addressing a different bug, where then removed it from the Tokenizer and doing the conversion in Template class. But that's not always the entry point to the Tokenizer. Note that in Core the templates are subclasses of Liquid::Document not Liquid::Template.

My question is more around doing double conversion to_s.to_str which I'm not sure I understand yet. I've double checked the documents and they're essentially alias of each other

From the docs

to_s → str
Returns self.

If called on a subclass of String, converts the receiver to a [String](https://devdocs.io/ruby~3/string) object.

Also aliased as: to_str
Returns self.

If called on a subclass of [String](https://devdocs.io/ruby~3/string), converts the receiver to a String object.

Alias for: to_s

I'm asking Ruby folks to see what I may be missing. Will address in follow up as soon as I hear back and learn that we legitimately need the double conversion.

@bahar-p bahar-p requested review from irenenam and mbarak and removed request for mbarak and irenenam December 17, 2024 16:37
@bahar-p bahar-p merged commit c3ac0e0 into main Dec 17, 2024
11 checks passed
@bahar-p bahar-p mentioned this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants