Clean up build-time API calls to Open Processing

[clairep94]

Cleanup post-resolution of failed builds (edited on May 14th):

Following the patch-fix to resolve builds hitting OP rate limits and discussions around updates to the api/sketch/[id] and api/curation/sketches, there are a few follow-up tasks for final cleanup:

1. OP: Update the data returned for the following endpoints in:

// `/api/sketch/:id`
fullname: string

// `/api/curation/:curationId/sketches`
licence: string

2. p5.js-website: Clean up getSketch to remove license: '' from below:

//https://github.com/processing/p5.js-website/blob/main/src/api/OpenProcessing.ts#L90-L94
if (memoizedSketch) {
      return {
        ...memoizedSketch,
        license: "", // take this out
      } as OpenProcessingSketchResponse;
    }

3. p5.js-website: Decouple SketchLayout and SketchesLayout by removing authorName from SketchLayout props

// intended change on https://github.com/processing/p5.js-website/blob/main/src/layouts/SketchLayout.astro#L18-L24

interface Props {
  sketchId: string;
}

const { sketchId } = Astro.props;
const { title, createdOn, instructions, fullname: authorName } = await getSketch(sketchId);

4. p5.js-website: Update any types described on https://github.com/processing/p5.js-website/blob/main/src/api/OpenProcessing.ts

---

Updated issue (last edited May 5th):

After further investigating the original issue below, it seems that the source of PRs failing test in the past week is that when the static site build is happening, and the project is trying to fetch sketch-data from OpenProcessing API, it is hitting 429 Too Many Requests

I've added error logging on data-fetching functions on PR #832, which still fail as expected, but show us the 429 status and error message below.

I have created a patch-fix solution on my fork here, which passes tests/build but we can see in the call-stack that after generating a certain amount of pages for individual sketches, we hit the limit on calls to OpenProcessing API again (please see PR details for screenshots & proposed solutions).

This leads to me believe that the solution may be to copy this issue to the OpenProcessing API repo, which seems to be private, and see if the rate-limit can be raised by 25 hits/10 minutes when being hit for site-builds. I have also proposed a second solution on clairep94#2, which could greatly reduce the amount of hits to OpenProcessing API that a build requires, but which would require some refactoring on the website & update to the API spec for getCurationSketches

Keen to hear everyone's thoughts!

---

Original Issue:

Potential bug with builds failing locally and on GHA when generating pages for some sketches

Most appropriate sections of the p5.js website?

Community (Sketches, Libraries, Events)

What is your operating system?

Mac OS

Web browser and version

Not an issue on the production site, but with build on GHA & locally

Actual Behavior

I noticed that some PR's created in the last week have been failing on a similar error to below, and the changes in the PRs seem unrelated to the errors.

I tried to build from main locally and encountered the same error, but with varying sketch ids.

I then tried to make a test PR with a nominal change (added a period in some homepage text), and encountered the same error, so potentially there may be something with the metadata of this sketch 2225254, or another source that is having a side-effect of this sketch page failing on build.

Affected PR	Failing Sketch ID & Stack Trace
#820	2225254
#821	2225254
#824	2225254
#826	2225254
#828	2225254
#830 (Test PR)	2225254

Expected Behavior

Build should not fail unless there is a breaking change on the branch
Test failures should vary between PRs given they are working on different issues

Steps to reproduce

• Pull from main & run npm run build locally, or make PR into main
• Might be good for someone to try this locally a few times to confirm if it is the same sketch ID that is failing each time, or if it varies.

Would you like to work on the issue?

Happy to look into it further or have it be assigned

[clairep94]

Hi @ksen0, tagging you for visibility! Let me know what you think

[ksen0]

Hey thanks @clairep94 ! Looking into it, will report back.

Edited to add: besides asking to change OP rate-limit, we can also potentially consider splitting build step so that fetching from OP API is not required for every single PR, but rather only when building main or dev-2.0

[msawired]

Hello 👋,
@ksen0 let me know about this issue. Yes, there are rateLimits on the API (currently 40 requests/min). I also looked at the code little, and I can see that multiple calls are made to 3 endpoints:

• api/curation/sketches
• api/sketch/[id]
• api/sketch/[id]/code

Looking at how the data is being used, I think it can be optimized to just use api/curation/sketches endpoint, as it contains all the data you need (including sketch and user metadata). I understand that p5js.org/sketches/[id] endpoint uses /api/sketch endpoint. To optimize this, you can lookup the initial curation/sketches data to find sketch metadata instead of making a new call to the API.

Not sure where "sketch/code" is used, as you are using iframes instead of rebuilding the whole sketch. Note that, this /code endpoint will require more strict authentication soon, to prevent webscraping efforts, etc. Possibly Bear Token.

Otherwise, I can also add specific IPs to whitelist to prevent rateLimiting, however that's not my preference, as some wrong code may hog the OP server and we wouldn't want that 😄...

[ksen0]

Thank you @msawired ! I agree that solutions that do not require changing rate limit should be fully investigated first and looks like removing the use of api/sketch/[id]/code and api/sketch/[id] (and rather storing / accessing original api/curation/sketches metadata) is both possible and would fix the current problem.

Potentially we can do temporary whitelisting to unblock merging, but I do think changing rate limit even temporarily can create more problems by accident, and I don't see critical PRs in the pipeline. So if this can be resolved n a few days, I think that could be alright.

@clairep94 what do you think / do you want to work on the fix?

[clairep94]

Hi both happy to work on the fix for the refactor! Will probably have some time this weekend

I think we might need to update api/curation/sketches to return some extra data that api/sketches/[id] was returning:

// from https://openprocessing.org/api/curation/87649/sketches
[
//...
{
    "visualID": "1957050",
    "title": "Generative Succulents",
    "description": "Generative succulents for the #WCCChallenge",
    "parentID": null,
    "thumbnailUpdatedOn": "2023-08-08 04:50:27",
    "videoUpdatedOn": null,
    "userID": "52944",
    "fullname": "newyellow",
    "membershipType": "1",
    "submittedOn": "2024-04-08 03:10:47",
    "status": "1"
  },
//...
]

// from the expected response for /api/sketch/[id]
// see https://github.com/processing/p5.js-website/blob/main/src/api/OpenProcessing.ts#L55-L68

export type OpenProcessingSketchResponse = {
  /** Sketch ID used for constructing URLs */
  visualID: string;
  /** Title of sketch */
  title: string;
  /** Description of sketch */
  description: string;
  instructions: string;
  license: string;
  userID: string;
  submittedOn: string;
  createdOn: string;
  mode: string;
};

@msawired would it be possible to add the following properties to items returned by the curation endpoint?

  instructions: string;
  createdOn: string;
  mode: string;

[ksen0]

@clairep94 thank you for working on this!

That's a good point that some information is missing. Do you think a temporary fix would be possible of e.g., building in a pause to stay under 40 requests/min? Or if removing /code one actually already resolves the immediate problem?

Though it would be good if the /sketches endpoint provided all info, I think this is critical enough that it would be good to patch our failing tests without having to wait for any changes in OP's API even if they are possible (and then, once the patch is merged to both main and 2.0 branches, coordinate on the more complete solution).

What do you think?

[msawired]

yes I can add those data points.
Are these API hits ultimately done on the frontend or on the backend?
If on the frontend, note that large networks such as schools may hit the rate limit if students visit the website at the same time, since they all come from the same IP, and whitelisting your servers wouldn't help. So, it is always good practice to keep the hits as minimum as needed.

[clairep94]

@ksen0

• I'll take a look to see if we can remove the /code hits & any other API-call optimisations we can do will solve the issue, then see if adding the pauses is necessary!

@msawired

• I believe the site is server-side generated, so the API hits are happening from the "backend" in the sense that the astro server is hitting the API, and creating static pages based on those results
• I am not the most experienced with SSG, so this might not be 100% correct -- @ksen0 would this be accurate?

Will have something for review likely tomorrow, thanks both :)

[limzykenneth]

The requests are happening on build so I guess we can consider it "backend"? It will run on each build and that includes all CI build step on each PR. It doesn't run on the built site served to users as the requested data are already built into the site.

@msawired One thing I'm thinking, not sure if there already is something like this already is a bulk request endpoint, so that we can pass a list of sketch ID and OP's API can return all the required data all in one round of request. That way we reduce the number of overall requests and it might be easier on your end for db queries as well if you can group that into one query too. We don't have to do it for this round of fix and can implement this later.

@clairep94 I think you all have a good handle on this and I'm just chiming in to confirm the above, but do ping me whenever if you need any clarifications or help.