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

Invalid JSON produced in reffy-reports #219

Closed
foolip opened this issue Jan 16, 2020 · 6 comments
Closed

Invalid JSON produced in reffy-reports #219

foolip opened this issue Jan 16, 2020 · 6 comments

Comments

@foolip
Copy link
Member

foolip commented Jan 16, 2020

w3c/webref@44cfbb4 broke ed/css/css-conditional.json in reffy-reports, making it no longer valid JSON.

This was the first update made using reffy commit 5414c43 and the recent changes then mighty have clues:
https://github.com/tidoust/reffy/commits/5414c43af99852c5b9dd228f01d7bb5a8730863e

#179 seems like the most plausible cause to me.

@tidoust

@foolip
Copy link
Member Author

foolip commented Jan 16, 2020

It's suspicious that the file is 634 bytes both before and after the change, and that the first 16 lines after the corruption are valid JSON. It looks a bit like only the first part of the file was modified. It doesn't look like that's the default behavior of https://nodejs.org/api/fs.html#fs_fs_writefile_file_data_options_callback however...

@tidoust
Copy link
Member

tidoust commented Jan 17, 2020

I'm a bit lost here. As far as I can tell, the ed/css/css-break.json file is a valid JSON file and its size is 3.01KB. What am I missing?

@tidoust
Copy link
Member

tidoust commented Jan 17, 2020

Ah, it's not css-break.json but rather css-conditional.json. I'll look into it.

@foolip
Copy link
Member Author

foolip commented Jan 17, 2020

Sorry, I pasted the wrong name!

@tidoust
Copy link
Member

tidoust commented Jan 17, 2020

What happens is the following:

  • Reffy crawls CSS Conditional Level 3 and CSS Conditional Level 4. It makes sense to crawl both specs because level 4 is currently written as a delta spec, so core definitions are in level 3.
  • Given that exported files do not include any level indication, when two specs lead to the same filename (here css-conditional), rule is to only export the latest level (that means we don't export all definitions, but it's hard to do anything better unless we add the notion of delta specs somewhere, see Handle "delta" specs in CSS dumps #117).
  • That rule works provided the code manages to determine which level is which. In this case, Level 3 still uses the "old" URL pattern with a css3-conditional shortname. Code fails to detect that this means level 3 and fails to detect that it is in the "two levels" situation.
  • We end up with two writeFile commands issued against the same file at the same time, which explains the weird output.

I'll make sure such clashes can no longer happen.

@foolip
Copy link
Member Author

foolip commented Jan 17, 2020

Yay, thanks for the detailed explanation and fix, @tidoust!

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

No branches or pull requests

2 participants