[Google Blockly] strip random block ids when saving xml #56299
Conversation
| function removeIdsFromBlocks(element) { | ||
| if (element.nodeName === 'block') { | ||
| const id = element.getAttribute('id'); | ||
| if (id && !Blockly.levelBlockIds.includes(id)) { |
There was a problem hiding this comment.
this function is only used in the context of google blockly right?
There was a problem hiding this comment.
Correct. And true of everything in this file!
|
I just added one more commit: c9c9475 The The reason for this is that loads in some block XML containing block ids which are needed to check the arrangement of the blocks. In practice, these blocks would have random ids which are now getting stripped out. To preserve the ids for the test, we can just add them to the toolbox and start blocks XML. |
|
🎉 Should we hold off on merging this until after the Sprite Lab migration? |
@ebeastlake I don't think it matters. We'll only be saving JSON for Sprite Lab (outside of levelbuilder functions), and never writing Sprite Lab solutions to the database. Edit: Let's hold this until after the migration! |
|
It might be good to check the production database before and after this is merged just to verify that we are indeed saving fewer unique solutions! |
|
@breville Agreed! That is the plan. |
Recently, there was concern about the potential for storing identical solutions in the
level_sourcestable in DB-backed levels for Google Blockly labs.@breville said:
He added:
Currently, this issue impacts DB-backed levels using Flappy, Bounce, and Dance. As it happens, Sprite Lab/Poetry are never DB-backed, always using S3 for project storage. Minecraft also uses the database for many levels, so it makes sense to address this before migrating that lab.
Stripping block ids at the time of saving is a reasonable step forward, however there are times when blocks need to have explicitly set ids. For example, block ids are used with callouts, automated tests, and possibly some older forms of Blockly code validation.
This branch creates a new property on the Blockly Wrapper that stores a list of block ids that were explicitly set in the level (start blocks or toolbox). Any other ids found can safely be considered randomly generated by Blockly and discarded.
Links
Testing story
To test this change, I solved a Minecraft puzzle the same way twice. I logged the XML before and after the stripping and observed the following:
when_runblock with idwhenRun.I had to get creative with the way the
appOptions.levelobject was passed and accessed due to failing automated tests. WhileappOptionsis typically available globally, this didn't prove true in the context of some are tests that needed to traverse these code changes.PR Checklist: