Skip to content

Defensive serving Scratch projects#922

Closed
abcampo-iry wants to merge 1 commit into
mainfrom
issues/1457-defensive-loading
Closed

Defensive serving Scratch projects#922
abcampo-iry wants to merge 1 commit into
mainfrom
issues/1457-defensive-loading

Conversation

@abcampo-iry

@abcampo-iry abcampo-iry commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Status

Points for consideration

  • Security: No change to authentication or authorization behaviour.
  • Performance: The project lookup now includes an inner join against scratch_components. This uses the existing indexed project_id and should have small to none impact.
  • Existing or historical Scratch projects without a Scratch component will now be treated as not found rather than causing an internal server error.

What’s changed?

  • Approached to fix Sentry issue.

  • Requires a Scratch project to have an associated Scratch component when loading it through the Scratch projects endpoint.

  • Returns the existing 404 Not Found response for projects in this invalid state instead of raising:

    NoMethodError: undefined method `content_with_stage_first` for nil
    
  • Adds a request spec covering a Scratch project without a Scratch component.

  • Valid Scratch projects are unaffected.

Steps to perform after deploying to production

None. This change does not require a migration, data cleanup, or Rake task. Existing malformed records will be handled safely by the endpoint.

@cla-bot cla-bot Bot added the cla-signed label Jul 13, 2026
@abcampo-iry abcampo-iry changed the title Defensive serving component Defensive serving Scratch projects Jul 13, 2026
@abcampo-iry
abcampo-iry marked this pull request as ready for review July 13, 2026 08:45
Copilot AI review requested due to automatic review settings July 13, 2026 08:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the Scratch projects API endpoint so malformed/historical Scratch projects (missing an associated scratch_component) are treated as 404 Not Found instead of raising a NoMethodError when rendering the Scratch JSON payload.

Changes:

  • Require a Scratch project to have an associated scratch_component when loading via the Scratch projects controller.
  • Add a request spec ensuring a Scratch project without a Scratch component returns 404 Not Found.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
spec/features/scratch/showing_a_scratch_project_spec.rb Adds coverage for the “missing scratch component returns 404” behavior.
app/controllers/api/scratch/projects_controller.rb Makes load_project enforce presence of scratch_component via an inner join, preventing nil dereference in show.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Test coverage

92.18% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/29267978882

@raspberrypiherokubot
raspberrypiherokubot temporarily deployed to editor-api-p-issues-145-64hffn July 13, 2026 08:50 Inactive
@abcampo-iry
abcampo-iry force-pushed the issues/1457-defensive-loading branch from 91823b6 to d74b1a2 Compare July 13, 2026 16:49

@zetter-rpf zetter-rpf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be other places now or in the future that the code loads scratch projects in the database so I didn't want to rely on this workaround alone.

I just found the one record that was causing the problem and deleted it, it was from a couple of months ago and was created by me so it might have been in that state because of a temporary issue or because we hadn't built all of the project saving yet.

Generally I think our system will be easier to work with if we fix data issues and prevent them from happening which will save us from dealing with data in unexpected shapes.

Can still merge this in if you think it would be useful to have.

@abcampo-iry

Copy link
Copy Markdown
Contributor Author

@zetter-rpf If we are not concerned anymore then this defensive change, it does make a small sense but is an extra join that doesn't really add much value. Happy to close

@abcampo-iry

Copy link
Copy Markdown
Contributor Author

Closing as we didn't see the error again, happy to open in the future if that's the case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants