-
Notifications
You must be signed in to change notification settings - Fork 62
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
fix: trust new notebooks and notebooks with new cells #275
Conversation
Creating a new notebook or adding a new cell to a notebook should not remove the trust status of the notebook.
Thank you @maartenbreddels! I have tried running this locally and I am running into a test failure for the Jupyter Packages Installed
Pytest Run Results
|
Hi @RRosio thank you for taking a look at this PR so thoroughly. Did you rebuild the JavaScript? A The test results you are looking at only runs the tests from the
Showing the test ran and passed. I hope this clarifies things. If not, I'm happy to assist further. Regards, Maarten |
Hi @maartenbreddels thank you for your reply and clarification! I completely forgot to rebuild the JS assets after checking out the branch! I did notice the Playwright Tests were actually running afterward but I was stuck on the local test failing. I've tried it out and it works great thank you!! |
@RRosio thank you so much for merging this PR -- it fixes a bug that's been driving us crazy for over a year, so it's really marvellous to have it available. :D |
@RRosio Thank you so much for merging this! Are you able to cut a new release containing this fix? Looks like there hasn't been an official nbclassic release since 1.0.0, which was released just over a year ago. |
Hi @RRosio, if I can help with anything related to making a release, please let me know. I'd love to see this high-friction fix released. |
Thank you for the feedback @jph00! |
Let me know if we can help at all with testing of the other PR and/or looking into CI failures. |
Thank you! I am taking a look at the |
The release is available now https://github.com/jupyter/nbclassic/releases/tag/v1.1.0! Please let me know if there is anything I missed or any issues! I will continue looking at the CI as well! |
Problem
When a new notebook cell is created from the front end (this includes creating a new notebook), the notebook is not trusted after a page reload. The core issue is that nbclassic creates an invalid notebook on the front end, which does not follow the nbformat 4.5 schema.
This problem only happens when Jupyter notebook 6.x/nbclassic is used in combination with nbformat >=5.1.0 (where nbformat schema 4.5 was introduced) or higher.
Before nbformat 5.1.0, there were no cell-ID's in the notebook format JEP 62.
Starting from nbformat 5.1.0, Cell-ID's are added to notebook data jupyter/nbformat#189.
Following JEP 62, raw_cell, markdown and code_cell should include an id field. When it is not present, nbformat will add the id field to the cell to make it valid according to the schema.
However, this happens after the notebook is 'trusted'. I am not an expert on the notary system, but mutation of the notebook after it is marked trusted will basically turn the notebook into an untrusted notebook.
Therefore, if we want to make use of the notary system as intended, we should send valid notebooks from the front end to the back end.
Steps to reproduce
Now, the notebook is seen as not trusted.
One could argue, this is not that bad, since it's a one time thing, but it is not: After trusting the notebook, add a new cell to the notebook, save, reload. The notebook is now again seen as untrusted.
Expected behavior
A new notebook and a notebook after a cell is added should be trusted after a reload.
Related issues / PRs
Solution
We should send valid notebooks from the front end to the back end. This means adding the id field to the raw_cell, markdown, and code_cell when they are created.
This PR is modeled after jupyterlab/jupyterlab#10018. Although the id field should only be set for the raw_cell, markdown and code_cell, the JupyterLab PR sets the id field for all cell types, we only do this for the raw_cell, markdown and code_cell as per the JEP 62.
As explained in the code, we use a uuid trimmed to 8 characters, like nbformat.