Conversation
A step's id is worked out from its name on every pull, and that conversion drops anything not url-safe. Two names that differ only in an emoji or an accent shorten to the same id, so the second step is written over the first. Two steps go in and one file comes out, with no warning. The file already holds each step's id and the reader already takes it as written. Only the pull side ignores it. So a pull now reads the ids the checked-out project has already given its steps and keeps them, deriving one only for a step it has not seen. A derived id that is taken gets a numeric suffix, and new steps are handled in uuid order so two people pulling the same project separately reach the same answer. Every name in a project today is ascii, so a kept id and a derived one are the same string and nothing moves for anyone. Edges named their target by deriving the target's name again. They now use the resolved id, so a reference cannot disagree with the step it points at. Ids are matched by name because the workflow file carries no uuids. That is why renaming a step still moves it. Tying an id to a step across a rename needs the uuid written down, which is a change to the file format and a separate question.
Review found three ways this could go wrong, two of them mine. An id read back from a project file went straight into a path. A workflow file can come from a repository somebody else prepared, so a step id of ../../.. was written outside the project. Deriving the id on every pull used to launder that; keeping it removed the only guard. Ids that are not already url-safe are now ignored. A step named constructor or toString found a function on Object.prototype, so both steps ended up sharing an id and one lost its body. That fired on an ordinary pull with nobody attacking anything. Ids are recorded per workflow rather than in one flat map, since two workflows can each hold a step of the same name, and both passes now run in uuid order so the answer does not depend on how the server listed the jobs. A trigger's id is its type and it shares the directory, so those are reserved before steps are given theirs. The map no longer leaks into the workspace config.
The hasOwnProperty call only mattered if a prototype member could pass isSafeId, and it cannot: a function is not a string. Mutating it away failed nothing, so it was a comment explaining code that did not need to be there.
The PR body told QA to expect a rename to move the step. That was a prediction from how the matching works, not something anyone had run.
Two workflows called `wf 👍` and `wf 👎` both slugify to `wf`, and serialize then throws `step not found`. So a pull crashes rather than quietly losing a step, and unlike the step case this one is reachable today, since workflow names have never been restricted. The id resolution is now shared, so a workflow keeps the id it already has and only gets a new one when it has not been seen. Recorded ids carry workflow ids alongside step ids, which is why the config field is now recordedIds.
packages/cli/.state.json is written by running the CLI locally.
7 tasks
Collaborator
|
But this is circular no? The step ids that were previously generated came on fetch from slugify. Preserving them is meaningless if they're not safe. And if they don't exist we still slugify them. So this just seems to be pushing the problem around and complicating the code. Also I think this is just locking in local IDs (even if step names change). There should be ways we can smartly and safely slugify to resolve conflict, I'd rather look at that. I was also hoping for this validation to come from lightning to avoid getting in this situation in the first place. Otherwise Lightning will also have to resolve this if eg we put step names in URLs. |
Collaborator
|
Closing as the linked issue OpenFn/lightning#5156 is a wontfix |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Short Description
Two steps called
step 👍andstep 👎both slugify tostep, so on pull one overwrites the other and a step's code is gone. Two workflows named that way both becomewf, and the pull crashes withstep not found. This keeps the ids a project already has instead of working them out again.Part of #1388, which asks for this directly: stop using slugify for identity.
Implementation Details
The file already stores each step's id, and the reader already takes it as written. Only the pull side ignores it. So a pull now keeps the ids the project has, and only works one out for a step it has not seen.
Nobody can hit the step case today because Lightning restricts step names to ASCII, but OpenFn/lightning#5106 lifts that. Workflow names have never been restricted, so that one is reachable now.
Ids are matched by name, since the files carry no uuids, so a rename still moves a step. There is a test for that.
QA Notes
Name two steps
step 👍andstep 👎and pull. You should get two.jsfiles with the right code in each. On main you get one.Same with two workflows. On main the pull throws.
AI Usage
You can read more details in our
Responsible AI Policy