Skip to content

Commit

Permalink
Fix for Issue OpenCyphal#104
Browse files Browse the repository at this point in the history 
This changes our logic for detecting duplicate definitions on case-sensitive file-systems to only operate on files of interest and to fix this detection logic per issue OpenCyphal#104.

If globular searches find duplicates we do not waste time detecting this until/unless these duplicates are actually considered when parsing target definitions.

# Conflicts:
#	pydsdl/_namespace.py
  • Loading branch information
@thirtytwobits
thirtytwobits committed Jun 28, 2024
1 parent c7d8ef9 commit 5bd1f46
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 96 deletions.
31 changes: 28 additions & 3 deletions pydsdl/_data_type_builder.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,18 @@ class MissingSerializationModeError(_error.InvalidDefinitionError):
pass


class DataTypeCollisionError(_error.InvalidDefinitionError):
"""
Raised when there are conflicting data type definitions.
"""


class DataTypeNameCollisionError(DataTypeCollisionError):
"""
Raised when type collisions are caused by naming conflicts.
"""


_logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -197,7 +209,11 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
_logger.debug("The full name of a relatively referred type %r reconstructed as %r", name, full_name)

del name
found = list(filter(lambda d: d.full_name == full_name and d.version == version, self._lookup_definitions))
found = list(
filter(
lambda d: d.full_name.lower() == full_name.lower() and d.version == version, self._lookup_definitions
)
)
if not found:
# Play Sherlock to help the user with mistakes like https://forum.opencyphal.org/t/904/2
requested_ns = full_name.split(_serializable.CompositeType.NAME_COMPONENT_SEPARATOR)[0]
Expand All @@ -218,8 +234,17 @@ def resolve_versioned_data_type(self, name: str, version: _serializable.Version)
error_description += " Please make sure that you specified the directories correctly."
raise UndefinedDataTypeError(error_description)

if len(found) > 1: # pragma: no cover
raise _error.InternalError("Conflicting definitions: %r" % found)
if len(found) > 1:
if (
found[0].full_name != found[1].full_name and found[0].full_name.lower() == found[1].full_name.lower()
): # pragma: no cover
# This only happens if the file system is case-sensitive.
raise DataTypeNameCollisionError(
"Full name of this definition differs from %s only by letter case, "
"which is not permitted" % found[0].file_path,
path=found[1].file_path,
)
raise DataTypeCollisionError("Conflicting definitions: %r" % found)

target_definition = found[0]
for visitor in self._definition_visitors:
Expand Down
85 changes: 16 additions & 69 deletions pydsdl/_namespace.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,18 +27,6 @@ class RootNamespaceNameCollisionError(_error.InvalidDefinitionError):
"""


class DataTypeCollisionError(_error.InvalidDefinitionError):
"""
Raised when there are conflicting data type definitions.
"""


class DataTypeNameCollisionError(DataTypeCollisionError):
"""
Raised when there are conflicting data type names.
"""


class NestedRootNamespaceError(_error.InvalidDefinitionError):
"""
Nested root namespaces are not allowed. This exception is thrown when this rule is violated.
Expand Down Expand Up @@ -260,9 +248,6 @@ def _complete_read_function(

lookup_dsdl_definitions = _construct_dsdl_definitions_from_namespaces(lookup_directories_path_list)

# Check for collisions against the lookup definitions also.
_ensure_no_collisions(target_dsdl_definitions, lookup_dsdl_definitions)

_logger.debug("Lookup DSDL definitions are listed below:")
for x in lookup_dsdl_definitions:
_logger.debug(_LOG_LIST_ITEM_PREFIX + str(x))
Expand Down Expand Up @@ -386,45 +371,7 @@ def _construct_dsdl_definitions_from_namespaces(
return dsdl_file_sort([_dsdl_definition.DSDLDefinition(*p) for p in source_file_paths])


def _ensure_no_collisions(
target_definitions: list[ReadableDSDLFile],
lookup_definitions: list[ReadableDSDLFile],
) -> None:
for tg in target_definitions:
tg_full_namespace_period = tg.full_namespace.lower() + "."
tg_full_name_period = tg.full_name.lower() + "."
for lu in lookup_definitions:
lu_full_namespace_period = lu.full_namespace.lower() + "."
lu_full_name_period = lu.full_name.lower() + "."
# This is to allow the following messages to coexist happily:
# zubax/non_colliding/iceberg/Ice.0.1.dsdl
# zubax/non_colliding/IceB.0.1.dsdl
# The following is still not allowed:
# zubax/colliding/iceberg/Ice.0.1.dsdl
# zubax/colliding/Iceberg.0.1.dsdl
if tg.full_name != lu.full_name and tg.full_name.lower() == lu.full_name.lower():
raise DataTypeNameCollisionError(
"Full name of this definition differs from %s only by letter case, "
"which is not permitted" % lu.file_path,
path=tg.file_path,
)
if (tg_full_namespace_period).startswith(lu_full_name_period):
raise DataTypeNameCollisionError(
"The namespace of this type conflicts with %s" % lu.file_path, path=tg.file_path
)
if (lu_full_namespace_period).startswith(tg_full_name_period):
raise DataTypeNameCollisionError(
"This type conflicts with the namespace of %s" % lu.file_path, path=tg.file_path
)
if (
tg_full_name_period == lu_full_name_period
and tg.version == lu.version
and not tg.file_path.samefile(lu.file_path)
): # https://github.com/OpenCyphal/pydsdl/issues/94
raise DataTypeCollisionError("This type is redefined in %s" % lu.file_path, path=tg.file_path)


def _ensure_no_fixed_port_id_collisions(types: list[_serializable.CompositeType]) -> None:
def _ensure_no_fixed_port_id_collisions(types: List[_serializable.CompositeType]) -> None:
for a in types:
for b in types:
different_names = a.full_name != b.full_name
Expand Down Expand Up @@ -864,22 +811,22 @@ def _unittest_read_files_empty_args() -> None:
assert len(transitive) == 0


def _unittest_ensure_no_collisions(temp_dsdl_factory) -> None: # type: ignore
from pytest import raises as expect_raises
def _unittest_ensure_no_namespace_name_collisions_or_nested_root_namespaces() -> None:
"""gratuitous coverage of the collision check where other tests don't cover some edge cases."""
_ensure_no_namespace_name_collisions_or_nested_root_namespaces([], False)

_ = temp_dsdl_factory

# gratuitous coverage of the collision check where other tests don't cover some edge cases
_ensure_no_namespace_name_collisions_or_nested_root_namespaces([], False)
def _unittest_issue_104(temp_dsdl_factory) -> None: # type: ignore
"""demonstrate that removing _ensure_no_collisions is okay"""

with expect_raises(DataTypeNameCollisionError):
_ensure_no_collisions(
[_dsdl_definition.DSDLDefinition(Path("a/b.1.0.dsdl"), Path("a"))],
[_dsdl_definition.DSDLDefinition(Path("a/B.1.0.dsdl"), Path("a"))],
)
thing_1_0 = Path("a/b/thing.1.0.dsdl")
thing_type_1_0 = Path("a/b/thing/thingtype.1.0.dsdl")

with expect_raises(DataTypeNameCollisionError):
_ensure_no_collisions(
[_dsdl_definition.DSDLDefinition(Path("a/b/c.1.0.dsdl"), Path("a"))],
[_dsdl_definition.DSDLDefinition(Path("a/b.1.0.dsdl"), Path("a"))],
)
file_at_root = temp_dsdl_factory.new_file(Path("a/Nothing.1.0.dsdl"), "@sealed\n")
thing_file = temp_dsdl_factory.new_file(thing_1_0, "@sealed\na.b.thing.thingtype.1.0 thing\n")
_ = temp_dsdl_factory.new_file(thing_type_1_0, "@sealed\n")

direct, transitive = read_files(thing_file, file_at_root.parent, file_at_root.parent)

assert len(direct) == 1
assert len(transitive) == 1
97 changes: 73 additions & 24 deletions pydsdl/_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from pathlib import Path
from textwrap import dedent
import pytest # This is only safe to import in test files!
from . import _data_type_builder
from . import _expression
from . import _error
from . import _parser
Expand All @@ -25,6 +26,8 @@
class Workspace:
def __init__(self) -> None:
self._tmp_dir = tempfile.TemporaryDirectory(prefix="pydsdl-test-")
name = self._tmp_dir.name.upper()
self._is_case_sensitive = not Path(name).exists()

@property
def directory(self) -> Path:
Expand Down Expand Up @@ -58,6 +61,10 @@ def drop(self, rel_path_glob: str) -> None:
for g in self.directory.glob(rel_path_glob):
g.unlink()

@property
def is_fs_case_sensitive(self) -> bool:
return self._is_case_sensitive


def parse_definition(
definition: _dsdl_definition.DSDLDefinition, lookup_definitions: Sequence[_dsdl_definition.DSDLDefinition]
Expand Down Expand Up @@ -1096,7 +1103,7 @@ def print_handler(d: Path, line: int, text: str) -> None:
"""
),
)
with raises(_namespace.DataTypeNameCollisionError):
with raises(_namespace.FixedPortIDCollisionError):
_namespace.read_namespace(
wrkspc.directory / "zubax",
[
Expand All @@ -1105,30 +1112,27 @@ def print_handler(d: Path, line: int, text: str) -> None:
)

# Do again to test single lookup-directory override
with raises(_namespace.DataTypeNameCollisionError):
with raises(_namespace.FixedPortIDCollisionError):
_namespace.read_namespace(wrkspc.directory / "zubax", wrkspc.directory / "zubax")

try:
(wrkspc.directory / "zubax/colliding/iceberg/300.Ice.30.0.dsdl").unlink()
wrkspc.new(
"zubax/COLLIDING/300.Iceberg.30.0.dsdl",
dedent(
"""
@extent 1024
---
@extent 1024
(wrkspc.directory / "zubax/colliding/iceberg/300.Ice.30.0.dsdl").unlink()
wrkspc.new(
"zubax/COLLIDING/300.Iceberg.30.0.dsdl",
dedent(
"""
),
)
with raises(_namespace.DataTypeNameCollisionError, match=".*letter case.*"):
_namespace.read_namespace(
@extent 1024
---
@extent 1024
"""
),
)
with raises(_namespace.FixedPortIDCollisionError):
_namespace.read_namespace(
wrkspc.directory / "zubax",
[
wrkspc.directory / "zubax",
[
wrkspc.directory / "zubax",
],
)
except _namespace.FixedPortIDCollisionError: # pragma: no cover
pass # We're running on a platform where paths are not case-sensitive.
],
)

# Test namespace can intersect with type name
(wrkspc.directory / "zubax/COLLIDING/300.Iceberg.30.0.dsdl").unlink()
Expand Down Expand Up @@ -1161,6 +1165,52 @@ def print_handler(d: Path, line: int, text: str) -> None:
assert "zubax.noncolliding.Iceb" in [x.full_name for x in parsed]


def _unittest_collision_on_case_sensitive_filesystem(wrkspc: Workspace) -> None:
from pytest import raises

if not wrkspc.is_fs_case_sensitive: # pragma: no cover
pytest.skip("This test is only relevant on case-sensitive filesystems.")

# Empty namespace.
assert [] == _namespace.read_namespace(wrkspc.directory)

wrkspc.new(
"atlantic/ships/Titanic.1.0.dsdl",
dedent(
"""
greenland.colliding.IceBerg.1.0[<=2] bergs
@sealed
"""
),
)

wrkspc.new(
"greenland/colliding/IceBerg.1.0.dsdl",
dedent(
"""
@sealed
"""
),
)

wrkspc.new(
"greenland/COLLIDING/IceBerg.1.0.dsdl",
dedent(
"""
@sealed
"""
),
)

with raises(_data_type_builder.DataTypeNameCollisionError, match=".*letter case.*"):
_namespace.read_namespace(
wrkspc.directory / "atlantic",
[
wrkspc.directory / "greenland",
],
)


def _unittest_parse_namespace_versioning(wrkspc: Workspace) -> None:
from pytest import raises

Expand Down Expand Up @@ -1265,8 +1315,7 @@ def _unittest_parse_namespace_versioning(wrkspc: Workspace) -> None:
),
)

with raises(_namespace.DataTypeCollisionError):
_namespace.read_namespace((wrkspc.directory / "ns"), [])
_namespace.read_namespace((wrkspc.directory / "ns"), [])

wrkspc.drop("ns/Spartans.30.2.dsdl")

Expand Down Expand Up @@ -1614,7 +1663,7 @@ def _unittest_issue94(wrkspc: Workspace) -> None:
wrkspc.new("outer_b/ns/Foo.1.0.dsdl", "@sealed") # Conflict!
wrkspc.new("outer_a/ns/Bar.1.0.dsdl", "Foo.1.0 fo\n@sealed") # Which Foo.1.0?

with raises(_namespace.DataTypeCollisionError):
with raises(_data_type_builder.DataTypeCollisionError):
_namespace.read_namespace(
wrkspc.directory / "outer_a" / "ns",
[wrkspc.directory / "outer_b" / "ns"],
Expand Down

0 comments on commit 5bd1f46

Please sign in to comment.