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.

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

Jump to bottom

Fix for Issue #104 #107

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Fix for Issue #104 #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1299df4
Fix for Issue #104
thirtytwobits Jun 27, 2024
d527ac1
Improving error handling
thirtytwobits Jul 11, 2024
d9c953a
Update pydsdl/_data_type_builder.py
thirtytwobits Jul 16, 2024
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
2 changes: 1 addition & 1 deletion pydsdl/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import sys as _sys
from pathlib import Path as _Path

__version__ = "1.21.0"
__version__ = "1.21.1"
__version_info__ = tuple(map(int, __version__.split(".")[:3]))
__license__ = "MIT"
__author__ = "OpenCyphal"
Expand Down
46 changes: 40 additions & 6 deletions pydsdl/_data_type_builder.py
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

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

This seems to make the search case-insensitive, why? In the Spec we state that identifiers are case-sensitive, but it is still considered a collision if identifiers differ by letter case only. Does this change allow the following:

# Foo.1.0.dsdl
bar.1.0 bar  # Will "bar.1.0" name "Bar.1.0"?
@sealed
# Bar.1.0.dsdl
@sealed

If yes, then it seems to be a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Bar.1.0.dsdl and bar.1.0.dsdl will both be accepted by this filter the later logic that requires only 1 result will raise the appropriate collision exception.

Copy link
Member

Choose a reason for hiding this comment

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

Okay but I'm not talking about collisions here, but rather the selectivity of the search. If I only have Bar.1.0.dsdl and I can name it bar.1.0.dsdl, we have a problem, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

You don't if only 1 dsdl file is found. If two are found then it is a collision caused either by case-sensitivity issues or by multiple paths to the same definition in different files. So selectivity doesn't matter since we never select one of multiple values returned from this filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

ping?

Copy link
Member

@pavel-kirienko pavel-kirienko Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This behavior is not spec-conformant. I understand that collision is a non-issue and am, in fact, talking about a completely different issue here: if the case doesn't match, the search should return nothing.

image

)
)
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,16 +234,34 @@ 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-insensitive.
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)
elif found[0].full_name != full_name and found[0].full_name.lower() == full_name.lower():
# pragma: no cover
# This only happens if the file system is case-sensitive.
raise DataTypeNameCollisionError(
"Full name of required definition %s differs from %s only by letter case, "
"which is not permitted" % (full_name, found[0].full_name),
path=found[0].file_path,
)

target_definition = found[0]
for visitor in self._definition_visitors:
visitor.on_definition(self._definition, target_definition)

assert isinstance(target_definition, ReadableDSDLFile)
assert target_definition.full_name == full_name
assert target_definition.version == version

for visitor in self._definition_visitors:
visitor.on_definition(self._definition, target_definition)

# Recursion is cool.
dt = target_definition.read(
lookup_definitions=self._lookup_definitions,
Expand Down
94 changes: 26 additions & 68 deletions pydsdl/_namespace.py
View file
Open in desktop
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,44 +371,6 @@ 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:
for a in types:
for b in types:
Expand Down Expand Up @@ -864,22 +811,33 @@ 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"))],
)
from pytest import raises

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"))],
)
thing_1_0 = Path("a/b/thing.1.0.dsdl")
thing_type_1_0 = Path("a/b/thing/thingtype.1.0.dsdl")

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

thing_1_1 = Path("a/b/thing.1.1.dsdl")

thing_file2 = temp_dsdl_factory.new_file(thing_1_1, "@sealed\na.b.thing.Thingtype.1.0 thing\n")

from ._data_type_builder import DataTypeNameCollisionError

with raises(DataTypeNameCollisionError):
read_files(thing_file2, file_at_root.parent, file_at_root.parent)
97 changes: 73 additions & 24 deletions pydsdl/_test.py
View file
Open in desktop
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
Loading