Fix check for relative mag paths in datasource validation #8720
Conversation
📝 WalkthroughWalkthroughThis change updates the logic for validating relative paths when editing dataset settings. The path validation in the data source service is now less strict, ensuring paths are checked relative to the organization and data source directories, rather than being overly restrictive. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🧰 Additional context used🪛 LanguageToolunreleased_changes/8720.md[duplication] ~1-~1: Possible typo: you repeated a word. (ENGLISH_WORD_REPEAT_RULE) ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| val path = organizationDir | ||
| .resolve(dataSource.id.directoryName) | ||
| .resolve(Path.of(new URI(pathStr).getPath).normalize()) | ||
| .toAbsolutePath |
There was a problem hiding this comment.
What about the directory of the layer? Like
val path = organizationDir
.resolve(dataSource.id.directoryName)
.resolve(dataLayerName)
.resolve(Path.of(new URI(pathStr).getPath).normalize())
.toAbsolutePath
There was a problem hiding this comment.
We decided recently with @normanrz that the mag paths should be relative to the dataset.
While it is true that some jsons still exist where they are relative to the layer, and wk still allows that, we want to change that soon (possibly migrating said jsons).
I’d say for this particular edge case it is no problem if the check is still too strict
There was a problem hiding this comment.
Just checked: Absolute and relative Paths. Both worked, but non-existent paths were accepted
Relative paths in datasource-properties.json are supposed to be interpreted as relative to the dataset directory. This particular codepath resolved them in the current working directory, which yields wrong results.
Note that this code will probably be changed soon again, when we make those paths (mostly) immutable.
Steps to test:
(I already tested locally)
..), should still be rejectedIssues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)