[JsonGen] Support wrapped on properties

[sebaszm]

• tests

[Copilot]

Pull request overview

Adds JsonGenerator support for explicitly wrapping JSON-RPC properties (via @wrapped), and introduces functional tests plus a test interface/implementation to validate wrapped vs unwrapped property behavior.

Changes:

• Update JsonGenerator result-shaping to allow @wrapped on properties and propagate property decorators between getter/setter.
• Extend the wrapped-interface functional test interface with Attribute (unwrapped) and AttributeWrapped (explicitly wrapped) properties.
• Add JSON-RPC functional tests for both wrapped and unwrapped property wire formats.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tests/FunctionalTests/jsonrpc/tests/TestWrappedInterfaceJsonRpc.cpp	Adds functional tests for wrapped vs unwrapped property JSON shapes.
tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h	Adds new property declarations used by functional tests and generator parsing.
tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp	Implements the new properties in the functional test implementation.
JsonGenerator/source/header_loader.py	Enables @wrapped behavior for properties and propagates decorators across property method pairs.

Suppressed comments (1)

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:59

• The output parameter tag looks misspelled as @ut; the JsonGenerator expects @out. With @ut, the parameter may not be treated as an output, breaking property getter generation.

        virtual Core::hresult AttributeWrapped(string& value /* @ut */) const = 0;

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

JsonGenerator Results

View Results

No changes detected.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

JsonGenerator/source/header_loader.py:1113

• Minor grammar issue in this new comment: it reads "Copy over decorators the other method..." and is missing "to".

            # Copy over decorators the other method of a property

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:65

• This setter is declared pure virtual (= 0) but has a body, and it assigns to _attribte (typo) which doesn't match the _attribute member. This will not compile and would not persist the value correctly.

        Core::hresult AttributeWrapped(const string& value) override = 0;
        {
            _attribte = value;
            return Core::ERROR_NONE;
        }

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:71

• This getter is declared pure virtual (= 0) but has a body, which is invalid C++. Remove the pure-virtual specifier so the class implements the interface method.

        Core::hresult AttributeWrapped(string& value) const override = 0;
        {
            value = _attribute;
            return Core::ERROR_NONE;
        }

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:59

• This method is declared as pure virtual (= 0) but also provides a definition, which is invalid C++ and will not compile. Remove the pure-virtual specifier so the class actually implements the interface getter.

        Core::hresult Attribute(string& value) const override = 0;
        {
            value = _attribute;
            return Core::ERROR_NONE;
        }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:53

• Grammar in the new property comment is missing an article and sentence terminator, which makes the interface documentation unclear.

This issue also appears on line 56 of the same file.

        // @property
        // A property is expected not to be wrapped even if interface is wrapped
        virtual Core::hresult Attribute(const string& value) = 0;

JsonGenerator/source/header_loader.py:1118

• Decorator propagation between the two methods of a property overwrites any decorators already present on the counterpart method and aliases the same list object. This can silently drop decorators (or make later changes affect both methods unexpectedly). Prefer merging decorators and copying the list.

            if method.retval.meta.decorators and method.retval.meta.is_property:
                for mm in face.obj.methods:
                    if mm != method and mm.name == method.name:
                        mm.retval.meta.decorators = method.retval.meta.decorators
                        break

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:59

• The output parameter annotation for AttributeWrapped getter is mistyped as @ut, so the generator may not recognize it as an output parameter (and property generation/validation can break).

        // @property @wrapped
        // Explicitly wrapped: expected to be wrapped even if a property
        virtual Core::hresult AttributeWrapped(const string& value) = 0;
        virtual Core::hresult AttributeWrapped(string& value /* @out */) const = 0;

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:58

• The new comment sentence is grammatically incomplete ("even if a property").

        // @property @wrapped
        // Explicitly wrapped: expected to be wrapped even if a property
        virtual Core::hresult AttributeWrapped(const string& value) = 0;

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

JsonGenerator/source/header_loader.py:1113

• Grammar: comment is missing the word "to" ("Copy over decorators to the other method...").

            # Copy over decorators the other method of a property

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:52

• Minor grammar: add "the" and a period to make the sentence read correctly.

        // @property
        // A property is expected not to be wrapped even if interface is wrapped

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:57

• Minor grammar: the sentence is incomplete ("even if a property"). Consider rewording to a complete statement.

        // @property @wrapped
        // Explicitly wrapped: expected to be wrapped even if a property

JsonGenerator/source/header_loader.py:1117

• Copying the decorators list by reference can unintentionally couple the two property methods if one side is modified later; make a shallow copy to avoid aliasing.

                    if mm != method and mm.name == method.name:
                        mm.retval.meta.decorators = method.retval.meta.decorators

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (6)

Previously missed (2) — in code that hasn't changed since the last review.

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:53

• Comment grammar: this sentence is missing an article and terminating period, which makes the intent harder to read in generated docs.

This issue also appears on line 56 of the same file.

        // @property
        // A property is expected not to be wrapped even if interface is wrapped
        virtual Core::hresult Attribute(const string& value) = 0;

JsonGenerator/source/header_loader.py:1118

• When copying property decorators between the getter/setter overloads, the current assignment overwrites any existing decorators on the other overload and also aliases the same decorators list between both methods. It’s safer to only populate the other overload when it has no decorators yet, and copy the list to avoid unintended shared mutation.

            if method.retval.meta.decorators and method.retval.meta.is_property:
                for mm in face.obj.methods:
                    if mm != method and mm.name == method.name:
                        mm.retval.meta.decorators = method.retval.meta.decorators
                        break

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:57

• Comment grammar: “even if a property” is an incomplete/unclear sentence fragment.

        // @property @wrapped
        // Explicitly wrapped: expected to be wrapped even if a property

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:71

• AttributeWrapped getter currently returns the Attribute backing field, so it can return the wrong value if Attribute was set last. Use a separate backing field for AttributeWrapped.

        Core::hresult AttributeWrapped(string& value) const override
        {
            value = _attribute;
            return Core::ERROR_NONE;
        }

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:78

• With separate backing storage for Attribute and AttributeWrapped, the implementation needs a second member field for AttributeWrapped.

    private:
        string _attribute;

tests/FunctionalTests/common/implementations/TestWrappedInterfaceImpl.cpp:65

• AttributeWrapped is implemented using the same backing field as Attribute, so setting one property changes the value returned by the other. These should be independent properties to avoid cross-test/state coupling.

        Core::hresult AttributeWrapped(const string& value) override
        {
            _attribute = value;
            return Core::ERROR_NONE;
        }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tests/FunctionalTests/jsonrpc/tests/TestWrappedInterfaceJsonRpc.cpp:60

• Typo in comment: "explictly" should be "explicitly".

    string response;
    // Interface wrapped and the property is explictly wrapped, so it does apply
    ASSERT_EQ(Core::ERROR_NONE, CallMethod("attributeWrapped", R"({"value":"pokus"})", response));

tests/FunctionalTests/common/interfaces/ITestWrappedInterface.h:59

• Same as Attribute(): please annotate the property setter parameter with /* @in */ for consistency with other FunctionalTests interfaces.

        // @property @wrapped
        // Explicitly wrapped: expected to be wrapped even if a property
        virtual Core::hresult AttributeWrapped(const string& value) = 0;
        virtual Core::hresult AttributeWrapped(string& value /* @out */) const = 0;