Skip to content

Strip non printable characters from locators #1720

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.

Sign up for GitHub

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

Merged
tjugdev merged 2 commits into from
Jun 16, 2026
Merged

Strip non printable characters from locators #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d349ac7
Strip non-printable characters when rendering locators
tjugdev Jun 15, 2026
01e2fc4
Add unit test
tjugdev Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Srclib/Types.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ module Srclib.Types (
) where

import Data.Aeson
import Data.Char qualified as Char
import Data.List.NonEmpty (NonEmpty ((:|)))
import Data.List.NonEmpty qualified as NE
import Data.Map (Map)
Expand Down Expand Up @@ -497,7 +498,10 @@ instance ToText Locator where

renderLocator :: Locator -> Text
renderLocator Locator{..} =
locatorFetcher <> "+" <> locatorProject <> "$" <> fromMaybe "" locatorRevision
stripNonPrintable $
locatorFetcher <> "+" <> locatorProject <> "$" <> fromMaybe "" locatorRevision
where
stripNonPrintable = Text.filter Char.isPrint

-- The projectId is the full locator of the project. E.g. custom+123/someProject (<fetcher>+<orgId>/<project-name>)
projectId :: Locator -> Text
Expand Down
6 changes: 6 additions & 0 deletions test/Srclib/TypesSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Srclib.Types (
SourceUnit (..),
SourceUnitBuild (..),
SourceUnitDependency (..),
renderLocator,
toProjectLocator,
translateSourceUnitLocators,
)
Expand All @@ -15,6 +16,11 @@ import Types (GraphBreadth (Complete))

spec :: Spec
spec = do
describe "renderLocator" $ do
it "should strip non-printable characters such as NUL bytes" $ do
let locator = Locator "go" "github.com/gin-gonic\NUL/gin" (Just "v1.\t9.1\DEL")
renderLocator locator `shouldBe` "go+github.com/gin-gonic/gin$v1.9.1"
Comment on lines +19 to +22

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial | 💤 Low value

Test correctly validates non-printable character stripping.

The test case covers the main use case from the PR objectives (NUL bytes in dependency names) and includes multiple control character types (\NUL, \t, \DEL). The implementation correctly verifies these are removed from the rendered locator string.

Optional enhancement: Consider adding test cases for edge cases such as a locator with no non-printable characters (regression safety) or fields that become empty after stripping, though the current coverage is sufficient for the acceptance criteria.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/Srclib/TypesSpec.hs` around lines 19 - 22, The renderLocator test
currently validates the main use case with multiple control characters but lacks
edge case coverage. Consider adding two additional test cases to the
renderLocator describe block: one test verifying that renderLocator correctly
handles a locator with no non-printable characters present (regression safety),
and another test covering the edge case where fields become empty or nearly
empty after stripping non-printable characters. These additions will improve the
robustness of the test suite.


describe "translateSourceUnitLocators" $ do
-- Helper to create a simple translation function from a map (for backward compatibility in tests)
let simpleTranslate translationMap loc =
Expand Down
Loading