Check for opaqueId before decode

[sujithvn]

Signed-off-by: Sujith [email protected]

Resolves #6680
Impact: minor
Type: feature

Issue

As mentioned in issue #6680, we have a requirement to remove "encoded ID" from all plugins. In the ADR it was decided that we were not going to be moving forward with any new work using Encoded ID, however across all the plugins it still exists and the inconsistency is probably going to create even more confusion. We should methodically go through each plugin and remove the necessity for using the encoded ID but also provide backwards compatibility for people who are currently using this

Solution

The plan is to deprecate the "encode/decode" in rel-5 and remove it in rel-6.
We have a new function isOpaqueId. This function uses a logic similar to what we have in decodeOpaqueId to check if the provided opaqueId is a valid by converting it and checking for namespace starting with string 'reaction/'.
This function is used to determine if provided id is opaque/encoded and if so, calls the decodeOpaqueId function, else uses the id as is. This should give indication that application does not need opaqueId whereas we still support them for backward compatibility during this time. In future development, it is expected that we do not use opaqueId at all.

As for the encoding part (while returning response from reaction), we have the encodeOpaqueId function, where we check the flag defined as config.REACTION_SHOULD_ENCODE_IDS and accordingly return original-Id or encoded-Id. I guess we could keep it as such since the user is expected to be aware/using this flag and expects the return values accordingly. But we mark it is deprecated and to be removed in rel6 where we return only original-Id.

Breaking changes

After discussion, config.REACTION_SHOULD_ENCODE_IDS set to false. This is a breaking change as preparation for complete removal of encode in v6. User can reset the flag back to true as a temporary fix in v5.

Testing

All existing test cases and new unit test should validate/pass.

[changeset-bot]

🦋 Changeset detected

Latest commit: 98f722e

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

This PR includes changesets to release 28 packages

Name	Type
reaction	Minor
@reactioncommerce/api-plugin-accounts	Minor
@reactioncommerce/api-plugin-address-validation	Minor
@reactioncommerce/api-plugin-authorization-simple	Minor
@reactioncommerce/api-plugin-carts	Minor
@reactioncommerce/api-plugin-catalogs	Minor
@reactioncommerce/api-plugin-discounts-codes	Minor
@reactioncommerce/api-plugin-email	Minor
@reactioncommerce/api-plugin-email-templates	Minor
@reactioncommerce/api-plugin-files	Minor
@reactioncommerce/api-plugin-inventory-simple	Minor
@reactioncommerce/api-plugin-navigation	Minor
@reactioncommerce/api-plugin-orders	Minor
@reactioncommerce/api-plugin-payments	Minor
@reactioncommerce/api-plugin-payments-stripe-sca	Minor
@reactioncommerce/api-plugin-pricing-simple	Minor
@reactioncommerce/api-plugin-products	Minor
@reactioncommerce/api-plugin-settings	Minor
@reactioncommerce/api-plugin-shipments	Minor
@reactioncommerce/api-plugin-shipments-flat-rate	Minor
@reactioncommerce/api-plugin-shops	Major
@reactioncommerce/api-plugin-sitemap-generator	Minor
@reactioncommerce/api-plugin-surcharges	Minor
@reactioncommerce/api-plugin-system-information	Minor
@reactioncommerce/api-plugin-tags	Minor
@reactioncommerce/api-plugin-taxes	Minor
@reactioncommerce/api-plugin-taxes-flat-rate	Minor
@reactioncommerce/api-utils	Minor

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

[tedraykov]

Let's add a @deprecated jsdocs tag to the decodeOpaqueId function:
Something like this:

/**
 * @name decodeOpaqueId
 * @method
 * @memberof GraphQL/Transforms
 * @deprecated In newer versions of reaction (v5 onwards) the encoding of IDs is not required. The encoding and decoding
 * of IDs will be removed in v6. If a developer implements a custom plugin they shouldn't encode and decode the IDs of
 * entities.
 * @summary Transforms an opaque ID to an internal ID
 * @param {String} opaqueId The ID to transform
 * @returns {String} An internal ID
 */
export default function decodeOpaqueId(opaqueId) {
 ...

Also in api-utils/lib/config.js let's change the default value of REACTION_SHOULD_ENCODE_IDS to false. This will cause a breaking change in v5 if the customers are using encoded ids in any way, but they will have a soft way of reverting to the previous behaviour be explicitly setting the env variable to true.

We should communicate this in the release notes.

[sujithvn]

Also in api-utils/lib/config.js let's change the default value of REACTION_SHOULD_ENCODE_IDS to false. This will cause a breaking change in v5 if the customers are using encoded ids in any way, but they will have a soft way of reverting to the previous behaviour be explicitly setting the env variable to true.

@tedraykov Thanks for the suggestions.
I have marked "deprecated" for the decodeOpaqueId function.
As for the the above comment related to updating config file, I do agree it is a 2-steps approach to make the transition and agree with it, but would request a confirmation from @zenweasel since this is an unplanned breaking change for v5.

@zenweasel - please confirm if this config file update as explained by Ted above is good to go.

[sujithvn]

This is a breaking change

After discussion, updated the REACTION_SHOULD_ENCODE_IDS to false.

[tedraykov]

Unfortunately, decoding and encoding are integrated into the unit and integration tests. We either refactor the tests or we revert the default REACTION_SHOULD_ENCODE_IDS to true. I would prefer the former but if you @sujithvn @zenweasel think we don't have resources for that we can also provide a {devDefault: testOnly(true)} so that the tests pass but the still keeping the default value to false

More context about testOnly:
https://github.com/af/envalid#testonly

[sujithvn]

FYI
Considered circleCI env vars also apart from the above suggestion from @tedraykov.
Since the above one from Ted help in local test also, decided for that

[aldeed]

Looks good. On the subject of the tests, technically you should probably wrap all the tests in .each and run them both with decoding and without it. If you're saying you support both ways, that would prove it.

However if you're going to go do all that refactoring, I'd personally just change everything to remove encoding and keep moving toward the future.