ZeligsoftDev / CX4CBDDS

CX4CBDDS component modeling and generation tool
Apache License 2.0
8 stars 5 forks source link

Regression in descriptors generation #428

Closed eposse closed 2 years ago

eposse commented 2 years ago

Issue and tracking information

If an assembly has nested parts and delegated ports, descriptor generation (from DeploymentPlans) raises an NPE.

See the attached model. axPubSub.zip

Developer's time Estimated effort to fix (hours):

Developer's Actual time spent on fix (hours)

Issue reporter to provide a detailed description of the issue in the space below

eposse commented 2 years ago

This was first observed in Issue #400. See this comment.

eposse commented 2 years ago

The regression occurred with the solution of Issue #397, as we didn't consider nested parts and delegate ports in that solution.

eposse commented 2 years ago

I've opened Pull Request #429. It prevents the NPE, but I think it doesn't fully solve the problem, as I think it doesn't filter certain properties that should be filtered if the defaults on the connector's properties are modified.

For example, in the model provided, there is a connector called "MyMessage_conn", and three deployments "SimpleApp", "ComplexApp" and "ReallyComplexApp". SimpleApp is like BasicPubSub. "ComplexApp" is similar, but now the publisher component ("MyPub") is nested inside an assembly ("MyPubContainer") with a delegated port. Finally, "ReallyComplexApp" is similar, but both the publisher and subscriber are nested inside assemblies with delegated ports.

Now, if in the SimpleApp deployment, I modify the "expression" in the QueryFilter of the pull_consumer and push_consumer, of the connector MyMessage_conn, then in the generated .cdp file, these configuration properties appear only on the subscriber, and not the publisher, as expected and agreed in Issue #397.

On the other hand, if in the ComplexApp or the ReallyComplexApp deployment I modify the same expression in the MyMessage_conn connector, then in the generated .cdp file, the configuration properties still appear on the publisher.

See the generated .cdp files attached.

The attached cdp files correspond to the three deployments. For each of them there are three: one with suffix "_nofilter_conn_defaults_modified" and the other with suffix "_filtered_conn_defaults_modified" and one with suffix "_filtered_conn_defaults_modified_expected". The "nofilter" versions were generated with filtering disabled, so all propertied are generated. The first "filter" versions are as they are currently generated with filtering enabled. The ones with the "_expected" suffix are the ones that I think should be generated, if my understanding is correct.

Can you confirm that those configuration properties should also be filtered in the nested publisher and that the files should be generated as shown in the files with the "_expected" suffix?

Thanks

depl.zip

emammoser commented 2 years ago

@eposse I think we should hold off on this PR until we have this fully solved. Yes, your expect CDP files are what I would expect to see. It may be helpful to also add in properties on the "_conn" like domain_id, qos_profile, or topic_name as those should not be filtered and should be available on both publishers and subscribers

eposse commented 2 years ago

I'll add those properties to double check they are not filtered.

I think I know what needs to be done to fix the nested cases. The commit in the PR just addresses the NPE, so we'll need it, but I'll only merge the PR once the other codegen cases are covered.

eposse commented 2 years ago

@j26151 I finally got it fixed. There should no longer be an NPE when using delegated ports and the CDP files correctly filter the properties.

You can get the build from here.

Let me know. Once this is checked, I can merge it into the branch for Issue #400.

emammoser commented 2 years ago

This looks good to me. I have verified I could generate descriptors for the models that I couldn't before and I compared the resulting CDP file to verify nothing looked out of place

eposse commented 2 years ago

Verified and approved by Paul as well. I've merged the PR, so we can close this one.