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

Output HEX-bytes instead of letters for ValidationNotEqualError. #5

Merged

Conversation

ams-tschoening
Copy link
Contributor

@ams-tschoening ams-tschoening commented Dec 7, 2020

That's what's done in Java as well and the current implementation pretty much reflects the same approaches implemented like in Java with the same formatted output. The following is an example error message in Java:

2020-12-02 13:24:12,501 ERROR
de.am_soft.util.backend.client.ClBase.callImpl: Exception occurred during method invocation.
org.apache.axis2.AxisFault: /seq/1: at pos 17: validation failed:
not equal, expected [2f 2f], but got [09 37]

And the following screenshots provide BEFORE and AFTER in Ruby:

Clipboard01

vs.

Clipboard01

C:/Users/TSCHOE~1/AppData/Local/Temp/d20201208-6932-1sqhtjx/par_oms_tpl_short_opt_aes_verify.rb:95:in `_read': /seq/1:
at pos 17: validation failed: not equal, expected [2f 2f], but got [09 37] (Kaitai::Struct::ValidationNotEqualError)

This fixes #4.

That's what's done in Java as well, though in Java individual bytes are separated by spaces as well. Am not doing so here to keep the implementation simple.

This fixes kaitai-io#4.
"byteArrayToHex" in Java, to have the same formatted output.
@generalmimon
Copy link
Member

generalmimon commented Dec 15, 2020

In general, please don't post console outputs as screenshots.

@generalmimon generalmimon self-requested a review December 15, 2020 12:38
@@ -577,7 +569,7 @@ def initialize(msg, io, src_path)
# "expected", but it turned out that it's not.
class ValidationNotEqualError < ValidationFailedError
def initialize(expected, actual, io, src_path)
super("not equal, expected #{self.class.strToHex(expected)}, but got #{self.class.strToHex(actual)}", io, src_path)
super("not equal, expected [#{Stream.format_hex(expected)}], but got [#{Stream.format_hex(actual)}]", io, src_path)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The brackets are by purpose to keep output somewhat in line with Java.

Copy link
Member

@generalmimon generalmimon left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

@generalmimon generalmimon merged commit 09b01b8 into kaitai-io:master Dec 15, 2020
@ams-tschoening ams-tschoening deleted the ghi_4_hex_bytes_vs_letters branch December 15, 2020 16:24
@@ -569,7 +569,7 @@ def initialize(msg, io, src_path)
# "expected", but it turned out that it's not.
class ValidationNotEqualError < ValidationFailedError
def initialize(expected, actual, io, src_path)
super("not equal, expected #{expected.inspect}, but got #{actual.inspect}", io, src_path)
super("not equal, expected [#{Stream.format_hex(expected)}], but got [#{Stream.format_hex(actual)}]", io, src_path)
Copy link
Member

@generalmimon generalmimon Mar 2, 2021

Choose a reason for hiding this comment

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

@ams-tschoening This comes with a severe oversight. The code does not work properly on anything except byte arrays (which are represented as strings in Ruby), which is reported by the failure of the test valid_fail_eq_int.ksy:

  2) ValidFailEqInt parses test properly
     Failure/Error:
       expect {
         r = ValidFailEqInt.from_file('src/fixed_struct.bin')
       }.to raise_error(Kaitai::Struct::ValidationNotEqualError)

       expected Kaitai::Struct::ValidationNotEqualError, got #<NoMethodError: undefined method `unpack' for 123:Integer> with backtrace:
         # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:532:in `format_hex'
         # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:572:in `initialize'
         # ./compiled/ruby/valid_fail_eq_int.rb:17:in `new'
         # ./compiled/ruby/valid_fail_eq_int.rb:17:in `_read'
         # ./compiled/ruby/valid_fail_eq_int.rb:12:in `initialize'
         # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:26:in `new'
         # C:/temp/kaitai_struct/runtime/ruby/lib/kaitai/struct/struct.rb:26:in `from_file'
         # ./spec/ruby/valid_fail_eq_int_spec.rb:8:in `block (3 levels) in <top (required)>'
         # ./spec/ruby/valid_fail_eq_int_spec.rb:7:in `block (2 levels) in <top (required)>'
     # ./spec/ruby/valid_fail_eq_int_spec.rb:7:in `block (2 levels) in <top (required)>'

Note that the 0.9 release comes with the valid key, which is actually a superset of contents (everything what can be done with contents can be done with valid, but not vice versa), so contents was internally unified with valid. More precisely, every usage of contents can be equivalently replaced with a byte array attribute using valid/eq and it will still produce the same code. For example, this snippet

seq:
  - id: a
    contents: [0x50, 0x4b]
  - id: b
    size: 2
    valid:
      eq: '[0x50, 0x4b]'

generates the following JavaScript code:

    this.a = this._io.readBytes(2);
    if (!((KaitaiStream.byteArrayCompare(this.a, [80, 75]) == 0))) {
      throw new KaitaiStream.ValidationNotEqualError([80, 75], this.a, this._io, "/seq/0");
    }
    this.b = this._io.readBytes(2);
    if (!((KaitaiStream.byteArrayCompare(this.b, [80, 75]) == 0))) {
      throw new KaitaiStream.ValidationNotEqualError([80, 75], this.b, this._io, "/seq/1");
    }

Nevertheless, the valid/eq and valid/any-of do not only work for byte arrays, but also for any type whose values can be compared by == (i.e. integer, float, string, byte array, enum), and valid/min and valid/max for anything that can be compared using < and > (again integer, float, string, byte array, but not enum) - see kaitai-io/kaitai_struct#435 and relevant test formats in kaitai_struct_tests (they begin with the valid_fail_ prefix).

And as you can see from the generated code above, the valid/eq key throws a ValidationNotEqualError if the check fails. So this error needs to work for all these types, not just byte arrays. For example, the error type ValidationNotEqualError in Java is defined like this (KaitaiStream.java:604-615):

    public static class ValidationNotEqualError extends ValidationFailedError {
        public ValidationNotEqualError(byte[] expected, byte[] actual, KaitaiStream io, String srcPath) {
            super("not equal, expected " + byteArrayToHex(expected) + ", but got " + byteArrayToHex(actual), io, srcPath);
        }

        public ValidationNotEqualError(Object expected, Object actual, KaitaiStream io, String srcPath) {
            super("not equal, expected " + expected + ", but got " + actual, io, srcPath);
        }

        protected Object expected;
        protected Object actual;
    }

Note that even though it treats byte arrays as special and dumps them using the byteArrayToHex() method, it also has a generic fallback for any other type (Object) which handles numbers and strings. That needs to be added to the Ruby runtime as well.

However, I'm not sure if it's possible to reliably distinguish text strings and byte strings apart at runtime - the both have the type String. There is a method String#encoding that tells ASCII-8BIT for byte strings and usually UTF-8 for text strings:

puts [0x50, 0x4b].pack('C*').class # => String
puts [0x50, 0x4b].pack('C*').encoding # => ASCII-8BIT
puts "text".class # => String
puts "text".encoding # => UTF-8

If the ASCII-8BIT is characteristic for byte strings, this looks like a way to go. From what I see in the generated Ruby code, a byte string is converted to a text string using the String#force_encoding method (see str_encodings.rb:15-24). The argument is the encoding identifier, which is pasted verbatim into the code by the compiler (see RubyTranslator.bytesToStr method. So for encoding: ASCII in the KSY spec (or manual .to_s("ASCII") invocation from within an expression), String#encoding tells US-ASCII, so that should be fine:

puts [0x50, 0x4b].pack('C*').force_encoding("ASCII").encoding # => US-ASCII

So unless one does encoding: ASCII-8BIT, which will definitely be excluded from the whitelist anyway once we put together a list of supported encodings (kaitai-io/kaitai_struct#116), it should work.

For now, I'll revert the change from this PR so that the Ruby runtime is not broken. Whenever you want to incorporate the change, open another PR with corrected implementation.

Copy link
Contributor Author

@ams-tschoening ams-tschoening Mar 3, 2021

Choose a reason for hiding this comment

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

Instead of guessing, how about simply making use of the fact that an exception is thrown? The following makes the mentioned test succeed for me:

  def initialize(expected, actual, io, src_path)
    begin
      super("not equal, expected [#{Stream.format_hex(expected)}], but got [#{Stream.format_hex(actual)}]", io, src_path)
    rescue  NoMethodError => e
      super("not equal, expected #{expected.inspect}, but got #{actual.inspect}", io, src_path)
    end

    @expected = expected
    @actual = actual
  end

generalmimon added a commit that referenced this pull request Mar 2, 2021
ams-tschoening added a commit to ams-ts-kaitai-runtime/ruby that referenced this pull request Mar 3, 2021
…tai-io#5)

That's what's done in Java as well and HEX-conversion was already available anyway. Bracktes are added by purpose to keep output somewhat compatible with that of Java.

This fixes kaitai-io#4.
ams-tschoening added a commit to ams-ts-kaitai-runtime/ruby that referenced this pull request Mar 3, 2021
generalmimon pushed a commit that referenced this pull request Apr 20, 2021
…rays. (#7)

* Output HEX-bytes instead of letters for ValidationNotEqualError. (#5)

That's what's done in Java as well and HEX-conversion was already available anyway. Bracktes are added by purpose to keep output somewhat compatible with that of Java.

This fixes #4.

* Use an implementation supporting other objects than byte arrays as well.

#5 (comment)

* Don't handle human readable texts as byte arrays.

* Rephrased docs to make more clear what is meant.

* Changed phrases.
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.

"Kaitai::Struct::ValidationNotEqualError" throws letters instead of HEX-bytes like in Java.
2 participants