Some selectors are not properly escaped while exported

BenjaminVanRyseghem commented 10 years ago

If you export Collections-Abstract, in Collection.class/methodsProperty the selector #\ is not properly escaped, the key is "\" where it should be "\\"

Then if you try to load it back, it fails since the character " is escaped. And the consequence is that the file structure is parsed wrong afterward

Step to reproduce

git clone https://github.com/BenjaminVanRyseghem/pharo-core.git

Then open Pharo from within the pharo-core folder, and evaluate:

| repo names |
repo := MCFileTreeRepository new directory: '.' asFileReference.

names := repo packageDescriptionsFromReadableFileNames.
names do: [ : name || version |
    version := repo versionFromFileNamed: name first, '.package'.
    [ version load ]
        on: MCMergeOrLoadWarning
        do: [ :ex | ex load ] ]

Same happens in Kernel.package/Integer.class, the key is "\" instead of "\". Exact same issue in Kernel.package/LargeInteger.class

dalehenrich commented 10 years ago

Ben,

Thanks for the report.

Which platform are you running on?

I cloned your repo and there is no Collection.class? When I look at the methodProperties.json file for Integer.class I see only

"\" : "bf 9/25/2008 15:13",

which looks correct as do the two entries in LargeInteger.class methodProperties:

"\" : "ClementBera 7/24/2013 17:03", "\" : "ClementBera 7/24/2013 17:03",

The error would be occurring in the json writer so I will take a peek and see if I can spot the error, but I am suspicious that something else is going wrong, because I do see properly written JSON in the above examples...

Dale

----- Original Message -----

| From: "Benjamin Van Ryseghem" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Sent: Monday, December 23, 2013 4:23:18 AM | Subject: [filetree] Some selectors are not properly escaped while | exported (#119)

| names := repo packageDescriptionsFromReadableFileNames. | names do: [ : name || version | | version := repo versionFromFileNamed: name first, '.package'. | [ version load ] | on: MCMergeOrLoadWarning | do: [ :ex | ex load ] ] | Same happens in Kernel.package/Integer.class , the key is "\" | instead of "\". | Exact same issue in Kernel.package/LargeInteger.class | — | Reply to this email directly or view it on GitHub .

BenjaminVanRyseghem commented 10 years ago

Still on Maverick.

The json is correct in the branch 3.0 since I use it for a jenkins build, so I fixed it by hand. If you check the incoming the problem is here

https://github.com/BenjaminVanRyseghem/pharo-core/blob/incoming/Collections-Abstract.package/Collection.class/methodProperties.json

(see how the syntax highlighter is failing, putting everything red ^^)

dalehenrich commented 10 years ago

Okay ... looked at the code and sure enough, there is no \ escaping going on ... I will try to push out a fix in the next week or so ...

Thanks for finding this...

Dale

----- Original Message -----

| From: "Benjamin Van Ryseghem" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, December 23, 2013 11:52:25 AM | Subject: Re: [filetree] Some selectors are not properly escaped while | exported (#119)

| Still on Maverick. | The json is correct in the branch 3.0 since I use it for a jenkins | build, so I fixed it by hand. | If you check the incoming the problem is here | https://github.com/BenjaminVanRyseghem/pharo-core/blob/incoming/Collections-Abstract.package/Collection.class/methodProperties.json | (see how the syntax highlighter is failing, putting everything red | ^^) | — | Reply to this email directly or view it on GitHub .

BenjaminVanRyseghem commented 10 years ago

Cool :)

It’s a pleasure to participate :P

I will try to have a look. If you have a pointer, I’ll take it :)

Ben

dalehenrich commented 10 years ago

Even Better:)

The error is in String>>writeCypressJsonOn:forHtml:indent: ... for cypress/filetree html forHtml is false and you will note that we're shipping strings with no checks for escapes whatsoever ... MCFileTreeJsonParser>>parseCharacter gives the list of legal escape characters. but I imagine that $\ is the only one that is required....

Dale

----- Original Message -----

| From: "Benjamin Van Ryseghem" notifications@github.com | To: "dalehenrich/filetree" filetree@noreply.github.com | Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com | Sent: Monday, December 23, 2013 12:01:43 PM | Subject: Re: [filetree] Some selectors are not properly escaped while | exported (#119)

| Cool :)

| It’s a pleasure to participate :P

| I will try to have a look. If you have a pointer, I’ll take it :)

| Ben | — | Reply to this email directly or view it on GitHub .

dalehenrich commented 10 years ago

and don't forget to add a test case in MCFileTreeJsonTest:)

Dale ----- Original Message -----

| Even Better:)

| The error is in String>>writeCypressJsonOn:forHtml:indent: ... for | cypress/filetree html forHtml is false and you will note that we're | shipping strings with no checks for escapes whatsoever ... | MCFileTreeJsonParser>>parseCharacter gives the list of legal escape | characters. but I imagine that $\ is the only one that is | required....

| Dale

| ----- Original Message -----

	From: "Benjamin Van Ryseghem" notifications@github.com
	To: "dalehenrich/filetree" filetree@noreply.github.com
	Cc: "Dale Henrichs" dale.henrichs@gemtalksystems.com
	Sent: Monday, December 23, 2013 12:01:43 PM
	Subject: Re: [filetree] Some selectors are not properly escaped
	while
	exported (#119)

	Cool :)

	It’s a pleasure to participate :P

	I will try to have a look. If you have a pointer, I’ll take it :)

	Ben
	—
	Reply to this email directly or view it on GitHub .

BenjaminVanRyseghem commented 10 years ago

See PR: https://github.com/dalehenrich/filetree/pull/120

Do not hesitate to teach me how you like your contributions :P

dalehenrich commented 9 years ago

PR #120 has been merged

dalehenrich / filetree

Some selectors are not properly escaped while exported #119