Skip to content
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

feat[core][Builder]: ENG-6337 replace query api with content api #3529

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

clyde-builderio
Copy link

@clyde-builderio clyde-builderio commented Sep 3, 2024

Description

Migrate gen1 SDK to use content API instead of query API

Jira
https://builder-io.atlassian.net/browse/ENG-6337

Copy link

changeset-bot bot commented Sep 3, 2024

🦋 Changeset detected

Latest commit: bdda5df

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@builder.io/sdk Major
@builder.io/react Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

gitguardian bot commented Sep 3, 2024

⚠️ GitGuardian has uncovered 15 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
9071768 Triggered Generic High Entropy Secret f7a29eb packages/sdks-tests/src/specs/symbol-with-repeat-input-binding.ts View secret
8530966 Triggered Generic High Entropy Secret 67edfcd nx.json View secret
2708648 Triggered Generic High Entropy Secret 67edfcd packages/sdks-tests/src/specs/large-reactive-state.ts View secret
2708648 Triggered Generic High Entropy Secret 67edfcd packages/sdks-tests/src/specs/index.ts View secret
2708648 Triggered Generic High Entropy Secret 67edfcd packages/sdks-tests/src/specs/large-reactive-state.ts View secret
11707119 Triggered Generic High Entropy Secret 67edfcd packages/sdks/snippets/react/src/routes/custom-components/advanced-child.tsx View secret
11707119 Triggered Generic High Entropy Secret 67edfcd packages/sdks/snippets/react/src/routes/custom-components/custom-child.tsx View secret
11707119 Triggered Generic High Entropy Secret 67edfcd packages/sdks/snippets/react/src/routes/custom-components/editable-region.tsx View secret
- - Generic High Entropy Secret 0e6981d packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 653fd7f packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 42dc345 packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 653fd7f packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 42dc345 packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 0e6981d packages/core/src/builder.class.test.ts View secret
- - Generic High Entropy Secret 653fd7f packages/core/src/builder.class.test.ts View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Contributor

@samijaber samijaber left a comment

Choose a reason for hiding this comment

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

3 things missing:

  • let's add a GetContentOptions config apiEndpoint: 'content' | 'query' that defaults to 'query' for backwards compatibility
  • run yarn g:changeset to create the changeset entry for this PR. Select both @builder.io/sdk and @builder.io/react in the CLI
  • let's add a test to the gen1 React SDK that makes sure this logic works.

To add the test:

  • add /get-content and /get-query paths to
    const PAGES = {
    '/': homepage,
    '/api-version-v1': CONTENT_WITHOUT_SYMBOLS,
    '/api-version-v3': CONTENT_WITHOUT_SYMBOLS,
    '/api-version-default': CONTENT_WITHOUT_SYMBOLS,
    '/can-track-false': homepage,
    '/css-nesting': cssNesting,
    '/columns': columns,
    '/symbols': symbols,
    '/js-code': JS_CODE_CONTENT,
    '/symbols-without-content': CONTENT_WITHOUT_SYMBOLS,
    '/symbol-bindings': symbolBindings,
    '/symbol-with-locale': symbolWithLocale,
    '/link-url': linkUrl,
    '/symbol-with-input-binding': symbolWithInputBinding,
    '/content-bindings': contentBindings,
    '/image': image,
    '/image-high-priority': imageHighPriority,
    '/image-no-webp': imageNoWebp,
    '/data-bindings': dataBindings,
    '/data-binding-styles': dataBindingStyles,
    '/react-native-strict-style-mode': REACT_NATIVE_STRICT_STYLE_MODE_CONTENT,
    '/react-native-strict-style-mode-disabled': REACT_NATIVE_STRICT_STYLE_MODE_CONTENT,
    '/ab-test': abTest,
    '/ab-test-interactive': AB_TEST_INTERACTIVE,
    '/http-requests': HTTP_REQUESTS,
    '/symbol-ab-test': symbolAbTest,
    '/custom-breakpoints': customBreakpoints,
    '/reactive-state': REACTIVE_STATE_CONTENT,
    '/large-reactive-state': LARGE_REACTIVE_STATE_CONTENT,
    '/element-events': elementEvents,
    '/external-data': EXTERNAL_DATA,
    '/show-hide-if': SHOW_HIDE_IF,
    '/show-hide-if-repeats': SHOW_HIDE_IF_REPEATS,
    '/custom-breakpoints-reset': customBreakpointsReset,
    '/text-block': textBlock,
    '/text-eval': textEval,
    '/state-binding': stateBinding,
    '/nested-symbols': nestedSymbols,
    '/editing-styles': EDITING_STYLES,
    '/video': video,
    '/repeat-items-bindings': REPEAT_ITEMS_BINDINGS,
    '/input-default-value': INPUT_DEFAULT_VALUE,
    '/duplicate-attributes': DUPLICATE_ATTRIBUTES,
    '/js-content-is-browser': JS_CONTENT_IS_BROWSER,
    '/slot': SLOT,
    '/slot-with-symbol': SLOT_WITH_SYMBOL,
    '/slot-without-symbol': SLOT_WITHOUT_SYMBOL,
    '/no-trusted-hosts': homepage,
    '/editing-styles-no-trusted-hosts': EDITING_STYLES,
    '/animations': ANIMATIONS,
    '/data-preview': DATA_PREVIEW,
    '/form': FORM,
    '/default-styles': DEFAULT_STYLES,
    '/css-properties': CSS_PROPERTIES,
    '/hover-animation': HOVER_ANIMATION,
    '/tabs': TABS,
    '/custom-components': CUSTOM_COMPONENTS,
    '/basic-styles': BASIC_STYLES,
    '/accordion': ACCORDION,
    '/accordion-one-at-a-time': ACCORDION_ONE_AT_A_TIME,
    '/accordion-grid': ACCORDION_GRID,
    '/symbol-tracking': SYMBOL_TRACKING,
    '/columns-with-different-widths': COLUMNS_WITH_DIFFERENT_WIDTHS,
    '/custom-components-models-show': CUSTOM_COMPONENTS_MODELS_RESTRICTION,
    '/custom-components-models-not-show': CUSTOM_COMPONENTS_MODELS_RESTRICTION,
    '/editing-box-columns-inner-layout': EDITING_BOX_TO_COLUMN_INNER_LAYOUT,
    } as const;
    , both pointing to the same asset (doesn't matter which, let's say homepage)
  • add /get-content as a switch case in
    switch (pathname) {
    case '/can-track-false':
    case '/symbol-tracking':
    extraProps = {
    canTrack: false,
    };
    break;
    case '/no-trusted-hosts':
    case '/editing-styles-no-trusted-hosts':
    extraProps = {
    trustedHosts: [],
    };
    break;
    case '/custom-components-models-show':
    // overrides page model below
    extraProps = {
    model: 'test-model',
    };
    break;
    case '/react-native-strict-style-mode':
    extraProps = {
    strictStyleMode: true,
    };
    break;
    default:
    break;
    }
    , so that it sets extraProps = { options: { apiEndpoint: 'content' } }
  • add playwright tests that open both routes (only for gen1 React SDK, use test.skip() to ignore other SDKs) and confirm that the requested URL is indeed api/v3/content and api/v3/query. you can see us doing that in other playwright tests like
    await page.route(urlMatch, route => {
    x++;
    if (x > 10) {
    throw new Error('Too many proxy API requests.');
    }
    return route.fulfill({
    status: 200,
    json: { entries: [{ seo_title: 'foo' }] },
    });
    });

packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
Comment on lines 2504 to 2510
if (areOptionsForCodegen) {
queryParams.options = queryParams.options || {};
queryParams.options[options.key!] = queryParams.options[options.key!] || {};
queryParams.options[options.key!][key] = JSON.stringify(value);
} else {
queryParams[key] = JSON.stringify(value);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you make this change? why does that logic only apply to codegen calls?

Copy link
Author

@clyde-builderio clyde-builderio Sep 5, 2024

Choose a reason for hiding this comment

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

In this thread (https://builder-internal.slack.com/archives/C01V9EH4T1N/p1725389926830789) Steve said says

yeah query string should be reusable, just note that there might be stuff in the old SDK to support multiple fetches in one response, so there may be causes where we are doing things like ?model.page.limit=10 that should just be ?limit=10 for /content
iirc they both go through the same code paths so even the old format may technically work in /content but cleaner to remove the model.{modelName}.key=value stuff

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see. For the sake of backwards compatibility, let's keep these options for both codegen and query endpoints, and remove them only if we're using the new content endpoint

packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@sidmohanty11 sidmohanty11 left a comment

Choose a reason for hiding this comment

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

I tested this out on some of my content. It is not able to parse the url (or its getting overridden / deleted somewhere) when I use it with

builder.get('page', { url: window.location.pathname, apiEndpoint: "content" }

but when I pass url inside options they work, so we might need to check that part first.

builder.get('page', { url: window.location.pathname, apiEndpoint: "content", options: { url: window.location.pathname } }

Screenshot 2024-09-11 at 3 12 02 PM

We might also need to test out content vs query on a whole different set of results. When I tested it with one of my reactivity content, it failed. It might be that the structure is very different (in how our React SDK is consuming it).

A side note: let's check the side effects of using content vs query when we use builder.getAll.

Turns out this needs very thorough testing before we go ahead and merge this. We also have not tested query filters, userAttributes, and other cases. Let's discuss this with @shyam-builder @rima-builder @sanyamkamat once on how we should go about this

packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
packages/core/src/builder.class.ts Show resolved Hide resolved
packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
packages/core/src/builder.class.ts Outdated Show resolved Hide resolved
Comment on lines +2534 to +2535
const isApiCallForCodegen = format === 'solid' || format === 'react';
const isApiCallForCodegenOrQuery = isApiCallForCodegen || apiEndpoint === 'query';
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move these out as constants and reuse them above?

packages/react-tests/react-vite/src/App.tsx Outdated Show resolved Hide resolved
.changeset/weak-bulldogs-exercise.md Outdated Show resolved Hide resolved
.changeset/weak-bulldogs-exercise.md Outdated Show resolved Hide resolved
Comment on lines +2537 to +2539
if (!isApiCallForCodegenOrQuery) {
delete queryParams.userAttributes;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this?

@@ -2363,6 +2372,8 @@ export class Builder {

const queue = useQueue || (usePastQueue ? this.priorContentQueue : this.getContentQueue) || [];

const apiEndpoint = queue[0].apiEndpoint || 'query';
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be such that we have different values for apiEndpoint in a queue? else can we move it out to a getter property?

@sidmohanty11
Copy link
Contributor

Just for context here, reactive bindings will require enrich true by default while using Content API
cc @samijaber

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants