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

fix(core): fix 'resolved vs unresolved' json path mapping #2202

Merged
merged 4 commits into from Jul 21, 2022

Conversation

padamstx
Copy link
Contributor

@padamstx padamstx commented Jul 6, 2022

This PR improves the DocumentInventory.findAssociatedItemForPath() function
by slightly changing the approach used to map a path from the resolved document
back to the corresponding path within the original unresolved document.
The result is a more accurate mapping due to the fact that the updated code
now more closely mimics the steps performed by the resolver when processing
the $refs, but just in reverse.

Fixes #2059
Fixes #2043

Checklist

  • Tests added / updated
  • Docs added / updated

Does this PR introduce a breaking change?

  • Yes
  • No

@padamstx padamstx requested a review from a team as a code owner July 6, 2022 23:27
@padamstx
Copy link
Contributor Author

padamstx commented Jul 7, 2022

@P0lip Hi Jakub, could you please give this PR a look when you get a chance? This fixes a few issues that we've been dealing with in the IBM openapi validator project (https://github.com/IBM/openapi-validator). Thanks!

@dpopp07
Copy link
Contributor

dpopp07 commented Jul 8, 2022

I think this represents an enormous correction in how paths are reported from the core Spectral tool. Would love to see this in a release soon!

@hudlow
Copy link

hudlow commented Jul 8, 2022

Thanks @padamstx! I confirmed this fixes #2043.

@P0lip
Copy link
Member

P0lip commented Jul 17, 2022

apologies for the delay folks, I was on vacation.
I'll be back tomorrow and will look at this PR as my first thing in the morning.

Copy link
Member

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Looks great, huge thanks for this one.
Since you've been waiting so long for the review, I didn't want my review to block it, thus I went ahead and added a few minor changes, mostly to use some utils we already have elsewhere in our libs.

commit a83422bae9b3dcf83e4604eb7c109bda0621872a
Author: Jakub Rożek <jakub@stoplight.io>
Date:   Mon Jul 18 09:01:12 2022 +0200

    refactor(core): use stoplight/json utils

diff --git a/packages/core/src/documentInventory.ts b/packages/core/src/documentInventory.ts
index cdb68015..f449e981 100644
--- a/packages/core/src/documentInventory.ts
+++ b/packages/core/src/documentInventory.ts
@@ -1,4 +1,4 @@
-import { extractSourceFromRef, isLocalRef } from '@stoplight/json';
+import { decodePointerFragment, encodePointerFragment, extractSourceFromRef, isLocalRef } from '@stoplight/json';
 import { extname, resolve } from '@stoplight/path';
 import { Dictionary, IParserResult, JsonPath } from '@stoplight/types';
 import { isObjectLike } from 'lodash';
@@ -113,16 +113,17 @@ export class DocumentInventory implements IDocumentInventory {
       let resolvedDoc = this.document;
 
       // Add '#' on the beginning of "path" to simplify the logic below.
-      const adjustedPath: JsonPath = [...'#', ...path];
+      const adjustedPath: string[] = ['#', ...path.map(String)];
 
       // Walk through the segments of 'path' one at a time, looking for
       // json path locations containing a $ref.
       let refMapKey = '';
       for (const segment of adjustedPath) {
-        if (refMapKey.length) {
-          refMapKey = refMapKey.concat('/');
+        if (refMapKey.length > 0) {
+          refMapKey += '/';
         }
-        refMapKey = refMapKey.concat(segment.toString().replace(/\//g, '~1'));
+
+        refMapKey += encodePointerFragment(segment);
 
         // If our current refMapKey value is in fact a key in refMap,
         // then we'll "reverse-resolve" it by replacing refMapKey with
@@ -149,7 +150,7 @@ export class DocumentInventory implements IDocumentInventory {
 
             // Update "resolvedDoc" to reflect the new "source" value and make sure we found an actual document.
             const newResolvedDoc = source === this.document.source ? this.document : this.referencedDocuments[source];
-            if (newResolvedDoc === null || newResolvedDoc === undefined) {
+            if (newResolvedDoc === null || newResolvedDoc === void 0) {
               const item: DocumentInventoryItem = {
                 document: resolvedDoc,
                 path: getClosestJsonPath(resolvedDoc.data, path),
@@ -157,6 +158,7 @@ export class DocumentInventory implements IDocumentInventory {
               };
               return item;
             }
+
             resolvedDoc = newResolvedDoc;
 
             // Update "refMap" to reflect the new "source" value.
@@ -174,25 +176,17 @@ export class DocumentInventory implements IDocumentInventory {
         missingPropertyPath: [...closestPath, ...missingPropertyPath],
       };
       return item;
-    } catch (e) {
-      // console.warn(`Caught exception! e=${e}`);
+    } catch {
       return null;
     }
   }
 
   protected convertRefMapKeyToPath(refPath: string): JsonPath {
-    const jsonPath: JsonPath = [];
-
     if (refPath.startsWith('#/')) {
       refPath = refPath.slice(2);
     }
 
-    const pathSegments: string[] = refPath.split('/');
-    for (const pathSegment of pathSegments) {
-      jsonPath.push(pathSegment.replace('~1', '/'));
-    }
-
-    return jsonPath;
+    return refPath.split('/').map(decodePointerFragment);
   }
 
   protected parseResolveResult: Resolver['parseResolveResult'] = resolveOpts => {

I tried to push that commit, but it appears I'm getting (permission denied), so I'd appreciate it if you could apply the above git patch.

@dpopp07
Copy link
Contributor

dpopp07 commented Jul 20, 2022

@P0lip Thanks for putting together that patch. @padamstx is also gonna be out for a little while and gave me push access to his fork. I'll push the patch commit up today or tomorrow

@dpopp07
Copy link
Contributor

dpopp07 commented Jul 20, 2022

Okay @P0lip I pushed the commit - let me know if there are any other issues!

Copy link
Member

@P0lip P0lip left a comment

Choose a reason for hiding this comment

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

Thank you all!

@P0lip P0lip enabled auto-merge (squash) July 21, 2022 08:39
@P0lip P0lip merged commit 157ec59 into stoplightio:develop Jul 21, 2022
stoplight-bot pushed a commit that referenced this pull request Jul 21, 2022
# [@stoplight/spectral-cli-v6.4.2](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-cli-v6.4.1...@stoplight/spectral-cli-v6.4.2) (2022-07-21)

### Bug Fixes

* **core:** fix 'resolved vs unresolved' json path mapping ([#2202](#2202)) ([157ec59](157ec59))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-cli-v6.4.2 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

stoplight-bot pushed a commit that referenced this pull request Jul 21, 2022
# [@stoplight/spectral-core-v1.12.4](https://github.com/stoplightio/spectral/compare/@stoplight/spectral-core-v1.12.3...@stoplight/spectral-core-v1.12.4) (2022-07-21)

### Bug Fixes

* **core:** fix 'resolved vs unresolved' json path mapping ([#2202](#2202)) ([157ec59](157ec59))
@stoplight-bot
Copy link
Collaborator

🎉 This PR is included in version @stoplight/spectral-core-v1.12.4 🎉

The release is available on npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

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