[b90-beta] Bug in varsFromStrProp() and getStrProp()

[wolph42]

This is a bug that I discovered by accident in both b87 and b90. In my case this is a particularly critical bug.

Paste the following code into the chat to see the issue:

Code: Select all

<pre>
[h:varsFromStrProp("test=0000001000")]
[r:string(test)]

[r:string(getStrProp("test=0000001000", "test"))]

[h:varsFromStrProp("test=@0000001000")]
[r:string(substring(test,1))]
 

The third returns a value which is to be expected.

The first changes the value into a number (thus '1000') before you can do anything. This might seem minor, but the error i ran into was issues with tokenID's!! Sometimes token id conists solely out of numeric characters AND they start with 0000 (as was with me the case). It took me a while to figure this out, but the resulting error is obvious.

The second though is the real kicker, it resulted in '512' .For no obvious reason its interpreted as octal

Note that if you use anything else for the second case, e.g. 000019 it works correctly. (so it results in 000019)

Edit: just to be complete: the delete issue is still present when the 'Impersonate Panel' is open. (Takes roughly 2 seconds to delete one token, takes ages if you delete 1000 tokens.)

[aliasmask]

When inside a string property it's definitely interpreting anything starting with a 0 as octal. If you start it with # or 0x it will interpret as hex when using Wiki: getStrProp(). When using 8 or 9 in the string, it knows it's not octal and will interpret as a string.

I found this in reference to java integer literals:

Integer Literals
In Java, you may enter integer numbers in several formats:

1. As decimal numbers such as 1995, 51966. Negative decimal numbers such as -42 are actually expressions consisting of the integer literal with the unary negation operation -.
2. As octal numbers, using a leading 0 (zero) digit and one or more additional octal digits (digits between 0 and 7), such as 077. Octal numbers may evaluate to negative numbers; for example 037777777770 is actually the decimal value -8.
3. As hexadecimal numbers, using the form 0x (or 0X) followed by one or more hexadecimal digits (digits from 0 to 9, a to f or A to F). For example, 0xCAFEBABEL is the long integer 3405691582. Like octal numbers, hexadecimal literals may represent negative numbers.

It's just seems to be one of those quirks with using string properties, yeah jsons! But in this case since you're wanting to use the vars functions, this is of no use to you. Now, if we had varsFromJson, that would be awesome.

[wolph42]

I use this at the core of the BoT, where speed is essential. VarsFromStrProp is the fastest function by far to pass variables onto. Using json would be slower.
...and octal, indeed, was a bit late last night.

[Lee]

@wolph I checked this out an, unfortunately, these aren't bugs per se. Everything, works as intended under the hood.

Case #1: The parser evaluates results in the following hieararchy: jsons -> numbers -> strings. The numbers -> string portion utilizes a "standard" check which tries to convert a result to a # and if it fails, accepts the result as a string. So, when 0000001000 gets evaluated, it gets converted to a number. The only way around it is to insert a non-numerical character into the mix. Note that making changes in this area can have unforeseen, and undesirable consequences. The best approach to keep things stable is to intersperse your macro code with the aforementioned characters, and some extra parsing.

Case #2: As it is written, the source decodes a value (i.e. Integer.decode) in order to ascertain integer values from a string. What triggered this in your case was that your string started with a "0", which is the hint for octal decoding. Without more feedback from users, it is "dangerous" to touch this as we don't know how many users actually value this (intended?/unintended?) functionality. I'll let Azhrei, et. al, decide.

Personally, from what I've seen, BigDecimal (a Java data type) is predominantly used all over the code; so much so, that in my JSON rewrite, I explicitly convert to BigDecimal for numerical values. I could do the same here, and it will preserve the value without decoding, but let's hear what other people say first.

[wolph42]

Well in this particular case I would also be happy if the token ids would never be a number iow always contain at least 1 letter. Eg preceded by an "i" , but that too would be pretty invasive and a Azh call.

I doubt though that the octal conversion is used for anything in is particular case. It is at the least an undocumented feature.

Also these 'feautures' are not something for which these functions were created. They are intended to retrieve values from a list in which you earlier stored those same variables in earlier. These variables should under no circumstance ever change.
Should you want to do a string,decimal or octal conversion, then you should have dedicated functions for that.

So in terms of the purpose of these functions, these are definitely unwanted features aka bugs.

[CoveredInFish]

I would agree with wolph on all points.

[Lee]

Case #1 I think the generation of the GUID was written in the early parts of MT (maybe even using Java 1.4?) Since Java 5+, there are "better" methods of generating unique ids, and I think implementing this won't break backward compatibility.

Case #2 Granted, it had undesired results in your use case, but we can't say for certain why the author chose this method and what his/her intentions really were, seeing that MT grew organically over the years with volunteer contributions to the code. I'll do the patch that replaces the Integer retrieval method; but I'm busy for the whole day, so I'll do it when I get home later.

[wolph42]

Thanks for picking this up so quickly and providing a fix! Awesome!

Case #1 I think the generation of the GUID was written in the early parts of MT (maybe even using Java 1.4?) Since Java 5+, there are "better" methods of generating unique ids, and I think implementing this won't break backward compatibility.

Case #2 Granted, it had undesired results in your use case, but we can't say for certain why the author chose this method and what his/her intentions really were, seeing that MT grew organically over the years with volunteer contributions to the code. I'll do the patch that replaces the Integer retrieval method; but I'm busy for the whole day, so I'll do it when I get home later.

Case1: I would wait for Azh to give his words on this, I have no clue what the impact could be.
Case2: granted we cannot look inside the creators head, but we can look at what he created and how he named it: STRINGpropertyList, so not a decimal, binary, octal or hexadecimal-propertyList. Also this 'feature' is not documented which also hints to 'not intended'

Anyway, out of pure curiosity, can you name one application for this octal conversion 'feature' ?

[Lee]

Case #1: I meant that I was looking to implement it on my version, and not official MT. I too would prefer ids to not have the potential to be purely numerical. In terms of backward compatibility, regardless of whatever the GUID looks like, as long as it uniquely represents whatever it was allocated to, it should be fine; regardless of whether or not the methods used for generation were uniform (RPTools custom GUID + other Java built-in methods). The built-in methods are less likely to produce collisions (not that any framework designer would have need for such a massive id base), but, more importantly, produce ids that are not purely numerical.

Case #2: Well, on my part, I wouldn't presume intent based on how a method was named. The string data type in any language is the convenient type to represent (as you know) any arbitrary value, allowing for parsing, manipulation etc. While you might store a number as a string, it is fundamentally still a number; other people would have uses for this, if one stops and thinks about it for a bit.

Off the top of my head, to answer your curiosity, I'd store numerical values as strings as a user-facing feature (i.e. table views) and decode it to an actual integer when doing actual processing. If I were an optimization freak, I'd be an** about memory consumption and note that even empty strings are at least 40 bytes in length whereas integers have a fixed(? I forget my computer science) length of 32-bits or 4 bytes.

So if I had a table of values, the first character of each string would be a hint as to what it should be decoded into, i.e. 0 for octal, 0x, 0X, or # for hexadecimal etc. etc. You've already pointed out that MapTool's string functions were faster than its JSON implementation, so maybe someone would like to roll some mathematical macros based on this premise.

Edit:
Patch submitted. As noted in the linked page, output will still have leading zeroes excised.

[wolph42]

thanks for the patch....and I just ran into another issue with getSelected() same problem. It returns id '100000' for a token '00000100000' and *that* it should certainly NOT do. I even tried
[id = string(getSelected()] but its still immediatley converted to integer and thus the whole function is useless (for 0 leading numerical id's). So I'm all of a sudden a lot more in favour for another token id.system.

do you want me to make a seperate bug report?

edit: sigh.... :

 json.toList("[000010000]") = json.toList("[000010000]") = 4096

im trying to find workarounds for this issue, but I keep on hitting walls. It looks like everything makes this annoying conversion which means that numerical token id's with leading 0's are all bugged.

[aliasmask]

Token ids should be 32 characters long.

I have no problem with

Code: Select all

[H: a = json.append("","0000100000")]
[r: json.toList(a)]

[wolph42]

It's just an example but I can assure you this as I have constant issues retrieving token ids. Including this method.

[Lee]

@wolph Well you could, though I haven't implemented anything yet. It's something I plan to do when I tackle the MT's asset management system, which is something quite a bit further downstream. Since I haven't tested my theories yet, there's a possibility that I am completely wrong

Yeah, any numerical string that the parser spits out as a result during the interpretation process will be converted to a real number. Unfortunately, viewed in this light, it's not a bug, it's just that the current id system sometimes churns out numerical ids. I can't check right now, but is the same GUID generated for an asset? If not, then perhaps you can do a check for numerical id, and recreate the token to get another id until you get one that's not numerical. If there are other methods that can do this cycling more efficiently, I guess that's the way to go at the moment.

[wolph42]

Good tip I'll delve into it

[Azhrei]

Hm, interesting problem.

As Lee says, the GUID needs to be globally unique but there's no reason why the code couldn't turn the first character of every GUID into a letter, forcing it to be treated as a string in the macro language functions.

Remember that the original implementation was never meant to expose GUID's to the user! They were "internal use only". When the macro language came along it became convenient to expose them to the macro programmer.

I don't have any simple workarounds off the top of my head. It may be possible to use a URI instead (such as asset://...) and thus avoid the automatic conversion, but that particular one will automatically retrieve the image. I don't think there are any other (user-accessible) URI methods that would be useful.