oasis-open / cti-python-stix2

OASIS TC Open Repository: Python APIs for STIX 2
https://stix2.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
356 stars 113 forks source link

@Custom decorators should warn if defining a common property it already does for you #506

Open clenk opened 3 years ago

clenk commented 3 years ago

For example, this should raise a warning:

@CustomObject('x-animal', [
    ('created', TimestampProperty(required=True, precision='millisecond')),
    ('species', properties.StringProperty(required=True)),
    ('animal_class', properties.StringProperty()),
])
class Animal(object):

because created is a common property that the decorator already defines automatically.

maybe-sybr commented 2 years ago

When you say raise a warning, do you mean something like a UserWarning or an actual exception? I'm guessing the former, but just interested. What is the current expected behaviour when a user does something like this? Is the property provided by the user respected or is it overwritten by the library?

chisholm commented 2 years ago

Clenk isn't around for awhile to speak for himself, but I suspect he meant to just display a warning. I don't know if he intended a simple logging statement of some sort, or a warnings.warn() type of warning. Current behavior unfortunately isn't consistent: a clashing property could overwrite what the decorator defines, or it could be overwritten.

The implementation creates a property order, e.g. [some properties] + [your properties] + [more properties]. So if your property clashes with a property in the first group, yours will overwrite it. If it clashes with a property in the last group, it will be overwritten. So you could look at the implementation and infer what will happen, but property conflicts should be avoided. Depending on how you define the conflicting property, you could wind up with a noncompliant object.

maybe-sybr commented 2 years ago

Makes sense, thanks for the clarification @chisholm

clenk commented 2 years ago

Yes, @chisholm has the right of it. I was thinking of warnings.warn().