Creating a singleton instance of Contentful #5

bounteous17 · 2024-02-14T11:06:38Z

Description

Being able to instance a client and retrieve content from Contentful
Next PR would implement a generic output format after parsing CMS retrieved data no matter which provider is (Contentful, Strapim etc ...)

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

socket-security · 2024-02-14T11:09:55Z

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@types/[email protected]	Transitive: environment, filesystem, unsafe	`+49`	5.74 MB	types
npm/[email protected]	Transitive: environment, eval, filesystem, network, shell, unsafe	`+531`	126 MB	contentful-ecosystem
npm/[email protected]	environment, eval, filesystem, network	`+31`	4.88 MB	contentful-ecosystem
npm/[email protected]	environment, filesystem	`0`	78.7 kB	motdotla
npm/[email protected]	Transitive: environment, eval, filesystem, network, shell, unsafe	`+290`	62.5 MB	simenb
npm/[email protected]	environment, filesystem, network, shell	`+9`	5.98 MB	mikaelb
npm/[email protected]	environment, filesystem, unsafe Transitive: eval, network, shell	`+293`	62.8 MB	kul

View full report↗︎

socket-security · 2024-02-14T11:09:58Z

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/[email protected], npm/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

matyasjay

Looks good to me so far! One thing I would add is the type infers from generics instead explicit any.

I'm thinking of something like:

 public async getEntry<T = Record<string, unknown>>(entryId: string) {
    return await this.getEntryHarmonized<T>(
      () => this.getClientInstance().getEntry<T>(entryId),
      // The following might not even needed if we can infer `T` from the return type
      ({ fields }: { fields: T }) => ({ data: fields }),
    );
  }

This requires typed client instances. The above example is a high level abstraction of what actually needed here. For now, I'd say we can merge this as is.

inigomarquinez

I really like your approach, @bounteous17

I've just added a small suggestion about the tests, but we can do it in a future PR if you want.

__tests__/e2e/cms/contentful.test.ts

inigomarquinez · 2024-02-14T12:22:30Z

@UlisesGascon , perhaps your feedback is useful here related to the alerts reported by socket.dev related to some dependencies running post-install scripts.

UlisesGascon · 2024-02-14T12:58:38Z

@UlisesGascon , perhaps your feedback is useful here related to the alerts reported by socket.dev related to some dependencies running post-install scripts.

I reviewed the comment from the socket.dev bot, and here are my fast conclusions on the topic:

Regarding `[email protected]`

Seems like [email protected] is using the postinstall script to run patch-package@^8.0.0:

{
   "postinstall": "patch-package",
}

Seems like this might not be necessary to run as it will only solve broken dependencies:

patch-package lets app authors instantly make and keep fixes to npm dependencies. It's a vital band-aid for those of us living on the bleeding edge. patch-package

Resources

Regarding `[email protected]`

Seems like [email protected] is using the postinstall script to run a [js file].

{
  "postinstall": "node postinstall"
}

File Content

The file published can be found at npm, see as /spawn-sync/postinstall.js. Please don't use Github to source the file as it might defer from the final code published.

'use strict';

function onError(err) {
try {
  var str = '' + (err ? (err.stack || err.message || err) : 'null');
  require('fs').writeFileSync(__dirname + '/error.log', str);
} catch (ex) {
}
}
try {
var fs = require('fs');
var cp = require('child_process');
var REQUIRES_UPDATE = false;
var pkg = JSON.parse(fs.readFileSync(__dirname + '/package.json', 'utf8'));
if (cp.spawnSync || __dirname.indexOf('node_modules') === -1) {
  if(pkg.dependencies['try-thread-sleep']){
    delete pkg.dependencies['try-thread-sleep'];
    REQUIRES_UPDATE = true;
  }
} else {
  if(!pkg.dependencies['try-thread-sleep']){
    pkg.dependencies['try-thread-sleep'] = "^1.0.0";
    REQUIRES_UPDATE = true;
    console.log('Installing native dependencies (this may take up to a minute)');
  }
}
if (REQUIRES_UPDATE && __dirname.indexOf('node_modules') !== -1) {
  fs.writeFileSync(__dirname + '/package.json', JSON.stringify(pkg, null, '  '));
  cp.exec((process.env.npm_execpath ? ('"' + process.argv[0] + '" "' + process.env.npm_execpath + '"') : 'npm') +
          ' install --production', {
    cwd: __dirname
  }, function (err) {
    if (err) onError(err);
    process.exit(0);
  });
  setTimeout(function () {
    process.exit(0);
  }, 60000);
}
} catch (ex) {
onError(ex);
}

This script seems to be a workaround for environments where child_process.spawnSync is not available. It adds or removes the try-thread-sleep dependency based on the environment and updates the dependencies accordingly. This was introduced as subdependency by [email protected]

/agnostic-cms-harmonizer
└─┬ [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      └─┬ [email protected]
        └── [email protected]

Based on the official documentation. child_process.spawnSync was added to Node in v0.11.12 (10y ago). As this is a fallback the execution is not require.

Resources

Conclusion

Overall, both postscripts does not seem malicious or with known vulnerabilities (AFIK), but I will suggest to install them using --ignore-scripts flag in npm. But as this is a library that will be distributed, I am not sure if the end users will take care of this or not.

UlisesGascon

I only reviewed the dependencies and I left a comment with my analysis.

UlisesGascon · 2024-02-14T13:01:01Z

@SocketSecurity ignore npm/[email protected]
@SocketSecurity ignore npm/[email protected]

UlisesGascon

Not sure why, but I just realized that the package-lock.json is different on my machine.

Steps to reproduce:

(Optional) delete the node_modules
nvm use. In my case node v20.11.0 (npm v10.2.4)
npm i

And here is the discrepancy:

inigomarquinez · 2024-02-14T14:18:08Z

Conclusion

Overall, both postscripts does not seem malicious or with known vulnerabilities (AFIK), but I will suggest to install them using --ignore-scripts flag in npm. But as this is a library that will be distributed, I am not sure if the end users will take care of this or not.

Thank you so much @UlisesGascon !

matyasjay · 2024-02-14T14:20:01Z

Conclusion

Overall, both postscripts does not seem malicious or with known vulnerabilities (AFIK), but I will suggest to install them using --ignore-scripts flag in npm. But as this is a library that will be distributed, I am not sure if the end users will take care of this or not.

Hi @UlisesGascon, thanks for your time and valuable info! One thing here: --ignore-scripts might be not optimal but as I mentioned to @inigomarquinez internally, running CI with --no-optional flag would be sufficient to ignore the dependency since contentful-cli defined as optional.

matyasjay · 2024-02-14T14:25:24Z

Not sure why, but I just realized that the package-lock.json is different on my machine.

Steps to reproduce:

(Optional) delete the node_modules

nvm use. In my case node v20.11.0 (npm v10.2.4)

npm i

And here is the discrepancy:

This is the result of you are running raw npm i. However in CI, optional deps should be ignored hence no contentful-cli in the lock file. You can repro by running the install with --no-optional flag. Not sure how to resolve this tho.
Probably the best to explicitly flag, that it is the user's responsibility to have the CLI installed instead having it listed.

bounteous17 · 2024-02-15T07:48:19Z

Looks good to me so far! One thing I would add is the type infers from generics instead explicit any.

I'm thinking of something like:
 public async getEntry<T = Record<string, unknown>>(entryId: string) {
    return await this.getEntryHarmonized<T>(
      () => this.getClientInstance().getEntry<T>(entryId),
      // The following might not even needed if we can infer `T` from the return type
      ({ fields }: { fields: T }) => ({ data: fields }),
    );
  }
This requires typed client instances. The above example is a high level abstraction of what actually needed here. For now, I'd say we can merge this as is.

@matyasjay @inigomarquinez we would implement the abstract type for the different providers instance on another PR. For the moment let's leave it as an any

bounteous17 · 2024-02-15T07:54:16Z

Not sure why, but I just realized that the package-lock.json is different on my machine.

Steps to reproduce:

(Optional) delete the node_modules

nvm use. In my case node v20.11.0 (npm v10.2.4)

npm i

And here is the discrepancy:

@UlisesGascon At the beginning contentful-cli was part of the dependencies until I realized that it should be part of optionalDependencies, that's the fix: 1cf9820

inigomarquinez · 2024-02-16T11:35:27Z

@all-contributors please add @bounteous17 for code,test,doc
@all-contributors please add @matyasjay for review
@all-contributors please add @UlisesGascon for review

inigomarquinez · 2024-02-16T11:43:34Z

@all-contributors please add @UlisesGascon for review

allcontributors · 2024-02-16T11:43:37Z

@inigomarquinez

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 940

inigomarquinez · 2024-02-16T11:45:23Z

@all-contributors please add @UlisesGascon for review

allcontributors · 2024-02-16T11:45:32Z

@inigomarquinez

I've put up a pull request to add @UlisesGascon! 🎉

allcontributors · 2024-02-16T11:55:54Z

@inigomarquinez

I've put up a pull request to add @bounteous17! 🎉

I've put up a pull request to add @matyasjay! 🎉

We had trouble processing your request. Please try again later.

bounteous17 added 9 commits February 12, 2024 13:56

chore: instance prototype

99b64e5

refactor: enhance non agnostic cms class extensions

c2bf4a8

ci: main application file

74cccd2

refactor: implement agnostic client abstract methods instead of redefine

c17f4e6

refactor: some agnostic instance class methods protected

2bd00e4

test: jest e2e setup

d9974df

chore: get space entry contentful without parser

cbedfc1

chore: contentful e2e space exports

e0e572c

test: e2e update export parsed output data

2d05398

bounteous17 requested review from NuriaExtremadouro, inigomarquinez, pabloelisseo, habondavid, robertoHeCi and matyasjay February 14, 2024 11:06

bounteous17 self-assigned this Feb 14, 2024

matyasjay reviewed Feb 14, 2024

View reviewed changes

inigomarquinez approved these changes Feb 14, 2024

View reviewed changes

__tests__/e2e/cms/contentful.test.ts Show resolved Hide resolved

UlisesGascon self-requested a review February 14, 2024 12:25

UlisesGascon approved these changes Feb 14, 2024

View reviewed changes

UlisesGascon requested changes Feb 14, 2024

View reviewed changes

fix: remove contentful-cli from "dependencies" section

1cf9820

inigomarquinez approved these changes Feb 15, 2024

View reviewed changes

bounteous17 requested review from UlisesGascon and matyasjay February 15, 2024 08:03

matyasjay approved these changes Feb 15, 2024

View reviewed changes

bounteous17 merged commit 12159d0 into main Feb 15, 2024
2 checks passed

bounteous17 deleted the CROS-48-create-a-singleton-instance branch February 15, 2024 08:21

This was referenced Feb 16, 2024

docs: add Bounteous17 as a contributor for code, test, and doc #8

Merged

docs: add matyasjay as a contributor for review #9

Merged

allcontributors bot mentioned this pull request Feb 16, 2024

docs: add UlisesGascon as a contributor for review #10

Merged

matyasjay mentioned this pull request Feb 17, 2024

feat: add inferred types and generics to Contentful client #13

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating a singleton instance of Contentful #5

Creating a singleton instance of Contentful #5

bounteous17 commented Feb 14, 2024

socket-security bot commented Feb 14, 2024 •

edited

Loading

socket-security bot commented Feb 14, 2024 •

edited

Loading

matyasjay left a comment •

edited

Loading

inigomarquinez left a comment

inigomarquinez commented Feb 14, 2024

UlisesGascon commented Feb 14, 2024

UlisesGascon left a comment

UlisesGascon commented Feb 14, 2024

UlisesGascon left a comment

inigomarquinez commented Feb 14, 2024

Conclusion

matyasjay commented Feb 14, 2024

Conclusion

matyasjay commented Feb 14, 2024 •

edited

Loading

bounteous17 commented Feb 15, 2024

bounteous17 commented Feb 15, 2024

inigomarquinez commented Feb 16, 2024

inigomarquinez commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

inigomarquinez commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

Creating a singleton instance of Contentful #5

Creating a singleton instance of Contentful #5

Conversation

bounteous17 commented Feb 14, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

socket-security bot commented Feb 14, 2024 • edited Loading

socket-security bot commented Feb 14, 2024 • edited Loading

Next steps

matyasjay left a comment • edited Loading

Choose a reason for hiding this comment

inigomarquinez left a comment

Choose a reason for hiding this comment

inigomarquinez commented Feb 14, 2024

UlisesGascon commented Feb 14, 2024

Regarding [email protected]

Regarding [email protected]

Conclusion

UlisesGascon left a comment

Choose a reason for hiding this comment

UlisesGascon commented Feb 14, 2024

UlisesGascon left a comment

Choose a reason for hiding this comment

inigomarquinez commented Feb 14, 2024

Conclusion

matyasjay commented Feb 14, 2024

Conclusion

matyasjay commented Feb 14, 2024 • edited Loading

bounteous17 commented Feb 15, 2024

bounteous17 commented Feb 15, 2024

inigomarquinez commented Feb 16, 2024

inigomarquinez commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

inigomarquinez commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

allcontributors bot commented Feb 16, 2024

socket-security bot commented Feb 14, 2024 •

edited

Loading

socket-security bot commented Feb 14, 2024 •

edited

Loading

matyasjay left a comment •

edited

Loading

Regarding `[email protected]`

Regarding `[email protected]`

matyasjay commented Feb 14, 2024 •

edited

Loading