Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Review the inheritance architechture #157

Open
diraol opened this issue Sep 6, 2016 · 5 comments
Open

Review the inheritance architechture #157

diraol opened this issue Sep 6, 2016 · 5 comments

Comments

@diraol
Copy link
Contributor

diraol commented Sep 6, 2016

We have found a big problem regarding the inheritance between versions.

The first plan was to import the classes from a previous version if they haven't changed on the spec between both versions.
So, for example, on v0x01 we have:
v0x01/symmetric/echo_request.py

from pyof.v0x01.common.header import Header, Type
from pyof.v0x01.foundation.base import GenericMessage

class EchoRequest(GenericMessage):
    header = Header(message_type=Type.OFPT_ECHO_REQUEST, length=8)

As the message EchoRequest has the same structure on the version v0x02, we expected to reuse the class from v0x01, so we would have:
v0x02/symmetric/echo_request.py

from pyof.v0x01.symmetric.echo_request import EchoRequest

One of the problems is related to the value passed to the header. So, for example:

>>> from pyof.v0x01.symmetric.echo_request import EchoRequest as ER1
>>> from pyof.v0x02.symmetric.echo_request import EchoRequest as ER2
>>> # Now let's create one message for each class:
>>> msg1 = ER1()
>>> msg2 = ER2()
>>> # Now we want to see the openflow version of each message:
>>> msg1.header.version
UBInt8(1)
>>> msg2.header.version
UBInt8(1)
>>> # As we can see, the version on the message from v0x02 is '1',
>>> # because it is inherited from v0x01.

The same behavior would happen for any attribute that have changed between versions and also for any message that the Type has changed (because a new Type was inserted on the enum before its options).

Another problem is that if we try to create subclasses between versions, the subclass will loose the __ordered__ dict from its superclass, and we need that attribute.

All in all, we need to review how the inheritance for evolutive reuse can be done.

For now, we have decided to fully implement the versions without inheritance between versions.

@diraol
Copy link
Contributor Author

diraol commented Sep 6, 2016

Please @cemsbr, @abaruchi, @beraldoleal and @raphaelmcobe help improve the above content so it is absolutely clear to everyone.
If we let something pass here, on the future, when we came back to this issue, we won't remember everything and understand all the problems that need to be handled.

@cemsbr
Copy link
Contributor

cemsbr commented Sep 6, 2016

Thanks for the clarification, I haven't noticed the __ordered__ problem yet. In some issue I think I wrote that if an Enum or Bitmask is changed, we implement it all again and every class that uses them. If an unchanged class uses an updated Enum and we import the class from v0x01, it will use the v0x01 Enum (because it imports its own Enum). Thus, whenever a class, Enum, Bitmask, etc changes, we should re-implement it and whatever files that depends on it.

However, if we find a module that has no changes, I would consider creating the module file in v0x02 that only imports the v0x01 version so the user can import anything from pyof.v0x02 (I haven't found such cases yet and they are expected to happen after all v0x02 changes are implemented). But I'll analyze it carefully before trying not to duplicate code.

@diraol
Copy link
Contributor Author

diraol commented Dec 21, 2016

I've been thinking about this inheritance issue, and I still cannot see an easy and clean way of addressing it.

It may be addressed by the MetaStruct metaclass, with some logic evaluating the Parent class (cls.__bases__), looking for the __ordered__ attribute.

We have some cases to think about:

  1. Add a new attribute at the end of the new class;
  2. Remove an existing attribute from the parent class;
  3. Change an existing attribute from the parent class (but keeping it on the same position);
  4. Move an attribute from position A to position B;
  5. Add a new attribute before/after another existing attribute;
  6. Rename an existing attribute;

One possible solution, to keep our GenericStruct/GenericMessage classes as they are, is to just (deep)copy the __ordered__ attribute from the parent class (if it have that attribute) and modify the Child class attributes using decorators (@addAttribute(attr_name, attr_class), @addAttributeAt(attr_name, attr_class, index), @removeAttribute(name), @moveAttributeAfter(name, attr_ref_name), @renameAttribute(name, new_name), etc).

Another approach would be to forget about our "orderedDict" and create a new attribute that will be a list with the name of the class attributes in the expected order.

These are just some ideas and insights I have had while trying to solve this problem. More ideas are welcome and I think that we need to have a deeper discussion on this matter, since it is a critical issue related to the evolution of the library.

Cheers!

@cemsbr
Copy link
Contributor

cemsbr commented Dec 22, 2016 via email

@cemsbr
Copy link
Contributor

cemsbr commented Jan 11, 2017

Please, check https://github.com/kytos/python-openflow/wiki/Version-Inheritance. I've just added a solution to this problem among other ones.

diraol pushed a commit that referenced this issue Jan 11, 2017
This commit alters the metaclass "MetaStruct". Now it will consider the
existance of a `__ordered__` attribute of the base classes being used
and also will evaluate three new `private` class attributes:
  - remove_attributes;
  - rename_attributes;
  - insert_before.

With these three new attributes now we can have control of class
attributes modifications while inheriting from another class (based on
GenericStruct or GenericMessage).

Usage:

```python
    class MyClassA(GenericStruct):
        attr_0 = UBInt8()
        attr_a = UBInt16()
        attr_z = UBInt8()
        attr_c = UBInt8()

    class MyClassB(MyClassA):
        attr_c = UBInt32()  # This will update the attr_c from parent class
        attr_d = UBInt8()   # This will add a new attribute positioned befor attr_a
                            # according to the defined below
        attr_e = 8          # This will add a new attribute at the end of the list.

        _remove_attributes = ['attr_z']
        _rename_attributes = [('attr_0', 'attr_new_name')]
        _insert_attributes_before = {'attr_d': 'attr_a'}
```

The resulting dictionary of 'MyClassB' will be:

```python
    OrderedDict([('attr_new_name', pyof.foundation.basic_types.UBInt8),
                 ('attr_d', pyof.foundation.basic_types.UBInt8),
                 ('attr_a', pyof.foundation.basic_types.UBInt16),
                 ('attr_c', pyof.foundation.basic_types.UBInt32),
                 ('attr_e', int)])
```

Signed-off-by: Diego Rabatone Oliveira <[email protected]>
diraol pushed a commit that referenced this issue Jan 19, 2017
This commit alters the metaclass "MetaStruct". Now it will consider the
existance of a `__ordered__` attribute of the base classes being used
and also will evaluate three new `private` class attributes:
  - remove_attributes;
  - rename_attributes;
  - insert_before.

With these three new attributes now we can have control of class
attributes modifications while inheriting from another class (based on
GenericStruct or GenericMessage).

Usage:

```python
    class MyClassA(GenericStruct):
        attr_0 = UBInt8()
        attr_a = UBInt16()
        attr_z = UBInt8()
        attr_c = UBInt8()

    class MyClassB(MyClassA):
        attr_c = UBInt32()  # This will update the attr_c from parent class
        attr_d = UBInt8()   # This will add a new attribute positioned befor attr_a
                            # according to the defined below
        attr_e = 8          # This will add a new attribute at the end of the list.

        _remove_attributes = ['attr_z']
        _rename_attributes = [('attr_0', 'attr_new_name')]
        _insert_attributes_before = {'attr_d': 'attr_a'}
```

The resulting dictionary of 'MyClassB' will be:

```python
    OrderedDict([('attr_new_name', pyof.foundation.basic_types.UBInt8),
                 ('attr_d', pyof.foundation.basic_types.UBInt8),
                 ('attr_a', pyof.foundation.basic_types.UBInt16),
                 ('attr_c', pyof.foundation.basic_types.UBInt32),
                 ('attr_e', int)])
```

Signed-off-by: Diego Rabatone Oliveira <[email protected]>
diraol pushed a commit that referenced this issue Jan 19, 2017
This commit alters the metaclass "MetaStruct". Now it will consider the
existance of a `__ordered__` attribute of the base classes being used
and also will evaluate three new `private` class attributes:
  - remove_attributes;
  - rename_attributes;
  - insert_before.

With these three new attributes now we can have control of class
attributes modifications while inheriting from another class (based on
GenericStruct or GenericMessage).

Usage:

```python
    class MyClassA(GenericStruct):
        attr_0 = UBInt8()
        attr_a = UBInt16()
        attr_z = UBInt8()
        attr_c = UBInt8()

    class MyClassB(MyClassA):
        attr_c = UBInt32()  # This will update the attr_c from parent class
        attr_d = UBInt8()   # This will add a new attribute positioned befor attr_a
                            # according to the defined below
        attr_e = 8          # This will add a new attribute at the end of the list.

        _remove_attributes = ['attr_z']
        _rename_attributes = [('attr_0', 'attr_new_name')]
        _insert_attributes_before = {'attr_d': 'attr_a'}
```

The resulting dictionary of 'MyClassB' will be:

```python
    OrderedDict([('attr_new_name', pyof.foundation.basic_types.UBInt8),
                 ('attr_d', pyof.foundation.basic_types.UBInt8),
                 ('attr_a', pyof.foundation.basic_types.UBInt16),
                 ('attr_c', pyof.foundation.basic_types.UBInt32),
                 ('attr_e', int)])
```

Signed-off-by: Diego Rabatone Oliveira <[email protected]>
@beraldoleal beraldoleal added this to the 2017.1b3 milestone Mar 31, 2017
@beraldoleal beraldoleal modified the milestones: 2017.2b1, 2017.1b3 Jun 19, 2017
@beraldoleal beraldoleal removed this from the 2017.2 milestone Dec 15, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants