search

ZeligsoftDev / CX4CBDDS

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

Validation errors for Delegated CORBA Facets that connect to AMI4CCM Receptacles #462

Closed emammoser closed 1 year ago

emammoser commented 1 year ago

Issue and tracking information

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

When a CORBA Facet is delegated out to an assembly that connects to anAMI4CCM receptacle, the model has a validation issue which should not occur.

To Reproduce, take the AMI4CCM example and place the slave components inside an assembly with the facet delegated to the assembly. Add that assembly to the overall diagram and connect the State Manager receptacle to it.

eposse commented 1 year ago

I'm attaching a copy of my modified model, to be sure I'm reproducing it correctly.

The errors I get are shown in the screenshot below. Are those the validation errors that you get?

If these are the same validation issues, then the current solution is to select the facet port in the assembly (in my model it's slavesStateControlFacet), select the "Properties" view, select the "Profile" tab, select and unfold the "InterfacePort" stereotype, select the "isAsynchronous" property, change the value to true.

After this, there are no validation errors.

Of course, changing the "isAsynchronous" property this way is cumbersome, so perhaps we should add a field in the main Properties view for the component, but first, I want to make sure this is the problem for you.

AMI4CCMExample2.zip

delegation_errors

emammoser commented 1 year ago

The example that you supplied still had ATCD architectural context and we only work with AXCIOMA in papyrus (even though your team might support both). I have converted the model and uploaded it with the error that I am seeing to the cx-reproducers repo under bug/462. You will notice that there is only 1 error now and its on the connection to the delegated port. All of the connectors from the delegated port to the component are no longer throwing errors.

I know we could set all of the facet ports to Asynchronous (both on the slaves and the delegate port), however, we don't want to do that or have to do that. "isAsynchronous" is supposed to be completely hidden to the user per: https://github.com/ZeligsoftDev/CX4CBDDS/issues/356 If we ignore that field, the only thing we can choose is connector type. All Facets should always be CORBA4CCM. Facets should never be AMI4CCM as it doesn't make sense. AMI4CCM means that as a user of a service (receptacle) I want to be able to make requests asynchronously instead of synchronously (CORBA4CCM), however, the provider of the service (facet) will always reply to requests synchronously (I get a request/someone calls a method on my interface and I respond)

eposse commented 1 year ago

I got the updated model and see the validation error. I've read through #356 and have some questions. I got from #356 that AMI4CCM ports are automatically set to asynchronous.

1) From your description above, facets should always be CORBA4CCM. Should we enforce this, perhaps with a new validation rule or by construction? 2) Should a CORBA4CCM port be always synchronous? (it's isAsynchronous property set to false)? 3) If I understand correctly, in your scenario, as shown in the model, the receptacle is an AMI4CCM port (which is asynchronous) and it is connected to a delegate facet of an assembly which is a synchronous CORBA4CCM port and which is connected to synchronous CORBA4CCM facets in the monolithic components (the slaves) inside the assembly. Is this accurate? 4) Assuming this is accurate, what you are saying is that there should not be a validation error when connecting a synchronous CORBA4CCM facet to an asynchronous AMI4CCM receptacle? 5) If this is the case, should we still keep the validation rule that requires the "isAsynchronous" flag to be the same on both sides of a connector for other cases?

emammoser commented 1 year ago

From your description above, facets should always be CORBA4CCM. Should we enforce this, perhaps with a new validation rule or by construction?

I wouldn't say every facet has to be CORBA4CCM, although at the moment that is the only connector type that we have that make sense. I could see a future where something like gRPC is added as a connector type and we could use that as a facet type. I don't think we should add any extra validation rule for something that isn't necessarily true.

Should a CORBA4CCM port be always synchronous? (it's isAsynchronous property set to false)?

CORBA4CCM is always synchronous, correct. In #356 we came to the conclusion that isAsynchronous really doesn't matter anymore but that it was kept around since it was used for generation. AMI4CCM is always asynchronous, CORBA4CCM is always synchronous.

If I understand correctly, in your scenario, as shown in the model, the receptacle is an AMI4CCM port (which is asynchronous) and it is connected to a delegate facet of an assembly which is a synchronous CORBA4CCM port and which is connected to synchronous CORBA4CCM facets in the monolithic components (the slaves) inside the assembly. Is this accurate?

Yes, you are correct

Assuming this is accurate, what you are saying is that there should not be a validation error when connecting a synchronous CORBA4CCM facet to an asynchronous AMI4CCM receptacle?

Exactly, the only way to use an asynchronous AMI4CCM receptacle is to connect it to a CORBA4CCM facet. We have always done this. In fact, our original AMI4CCM example doesn't have this error because we have no assemblies with delegated ports. This is only occurring in this example because of the delegated CORBA4CCM port.

If this is the case, should we still keep the validation rule that requires the "isAsynchronous" flag to be the same on both sides of a connector for other cases?

I am not entirely sure here. At the end of the day, our team doesn't care about the "isAsynchronous" flag at all, it is more of an internal field that CX is using for book keeping and generation. We just want to be able to set receptacles to AMI4CCM, and facets to CORBA4CCM, and be able to delegate those interfaces as much as we need to in organizing our assemblies

jwillemsen commented 1 year ago

Should a CORBA4CCM port be always synchronous? (it's isAsynchronous property set to false)?

CORBA4CCM is always asynchronous, correct. In #356 we came to the conclusion that isAsynchronous really doesn't matter anymore but that it was kept around since it was used for generation. AMI4CCM is always asynchronous, CORBA4CCM is always synchronous.

I think you want to say it is always synchronous.

emammoser commented 1 year ago

I think you want to say it is always synchronous.

Yes, thank you, I edited the comment above to be correct.

eposse commented 1 year ago

An update and some questions.

It has taken me a while to work this one out because the current validation error is produced by a validation constraint of the CCM meta-model, so we needed a way to override this constraint, and this required some modifications to our validation constraints manager. The updates now work, but I want to make sure that the new validation rule correctly captures the different possible cases.

First, some terminology (correct me if I'm wrong):

So, could you confirm whether the following cases are deemed to be valid or invalid?

Valid: 1) Internal receptacle with (async) AMI4CCM connector type to internal facet with (sync) CORBA4CCM connector type 2) Internal receptacle with (sync) CORBA4CCM connector type to internal facet with (sync) CORBA4CCM connector type 3) Internal receptacle with (async) AMI4CCM connector type to boundary receptacle with (async) AMI4CCM connector type 4) Internal facet with (async) AMI4CCM connector type to boundary facet with (async) AMI4CCM connector type 5) Internal receptacle with (sync) CORBA4CCM connector type to boundary receptacle with (sync) CORBA4CCM connector type 6) Internal facet with (sync) CORBA4CCM connector type to boundary facet with (sync) CORBA4CCM connector type

Invalid: 7) Internal receptacle with (async) AMI4CCM connector type to internal facet with (async) AMI4CCM connector type 8) Internal receptacle with (sync) CORBA4CCM connector type to internal facet with (async) AMI4CCM connector type 9) Internal receptacle with (async) AMI4CCM connector type to boundary receptacle with (sync) CORBA4CCM connector type 10) Internal receptacle with (sync) CORBA4CCM connector type to boundary receptacle with (async) AMI4CCM connector type 11) Internal receptacle to internal receptacle 12) Internal facet to internal facet

Others valid/invalid? 13) Boundary to boundary?

To summarize it, as a table, with the following legend:

IRA: Internal Receptacle AMI4CCM IRC: Internal Receptacle CORBA4CCM IFA: Internal Facet AMI4CCM IFC: Internal Facet CORBA4CCM BRA: Boundary Receptacle AMI4CCM BRC: Boundary Receptacle CORBA4CCM BFA: Boundary Facet AMI4CCM BFC: Boundary Facet CORBA4CCM

Y: Yes (valid) N: No (invalid) ?: Don't know

The table below is symmetric w.r.t. the diagonal, so I'm omitting the lower half:

IRA IRC IFA IFC BRA BRC BFA BFC
IRA N N N Y Y N N N
IRC N N Y N Y N N
IFA N N N N Y N
IFC N N N N Y
BRA ? N N N
BRC ? N N
BFA ? N
BFC ?

Does this capture correctly all the valid and invalid cases?

Note that the unknown entries in the diagonal are boundary-to-boundary connectors. Are they legal use cases? How should we treat them?

emammoser commented 1 year ago

Valid:

  1. Internal receptacle with (async) AMI4CCM connector type to internal facet with (sync) CORBA4CCM connector type
  2. Internal receptacle with (sync) CORBA4CCM connector type to internal facet with (sync) CORBA4CCM connector type
  3. Internal receptacle with (async) AMI4CCM connector type to boundary receptacle with (async) AMI4CCM connector type
  4. Internal facet with (async) AMI4CCM connector type to boundary facet with (async) AMI4CCM connector type
  5. Internal receptacle with (sync) CORBA4CCM connector type to boundary receptacle with (sync) CORBA4CCM connector type
  6. Internal facet with (sync) CORBA4CCM connector type to boundary facet with (sync) CORBA4CCM connector type

Invalid: 7) Internal receptacle with (async) AMI4CCM connector type to internal facet with (async) AMI4CCM connector type 8) Internal receptacle with (sync) CORBA4CCM connector type to internal facet with (async) AMI4CCM connector type 9) Internal receptacle with (async) AMI4CCM connector type to boundary receptacle with (sync) CORBA4CCM connector type 10) Internal receptacle with (sync) CORBA4CCM connector type to boundary receptacle with (async) AMI4CCM connector type 11) Internal receptacle to internal receptacle 12) Internal facet to internal facet

I would say #4,7,8 is invalid solely because you cannot have any facets with AMI4CCM.

9 & 10 are interesting in that I don't think we really care or need to enforce that. I could see an edge case where we have multiple components with different internal receptacles (some being CORBA4CCM, some being AMI4CCM) connecting to a boundary receptacle (AMI4CCM/CORBA4CCM) and we don't really care or want to enforce anything there because the assembly itself won't be generated to anything in the CDP. That being said I am 100% okay with having #9 & #10 be invalid to keep the modeling and validation simpler and if such a scenario above did happen, the user can just model two separate boundary ports, one AMI4CCM, one CORBA4CCM, and connect the respective internal receptacles to their respective boundary ports.

What would a boundary-to-boundary connector be? Since you state that you "do not refer to the instance of 'slavesStateControlFacet' in the top-level AMI4CCM_asm assembly as a boundary port", would a boundary-to-boundary connector only exist in a single assembly diagram where you connect one boundary (delegate) to another boundary (delegate) on the same assembly? That wouldn't make sense as we would just connect the internal ports themselves together or just model it differently.

eposse commented 1 year ago

9 & 10 are interesting in that I don't think we really care or need to enforce that. I could see an edge case where we have multiple components with different internal receptacles (some being CORBA4CCM, some being AMI4CCM) connecting to a boundary receptacle (AMI4CCM/CORBA4CCM) and we don't really care or want to enforce anything there because the assembly itself won't be generated to anything in the CDP. That being said I am 100% okay with having #9 & #10 be invalid to keep the modeling and validation simpler and if such a scenario above did happen, the user can just model two separate boundary ports, one AMI4CCM, one CORBA4CCM, and connect the respective internal receptacles to their respective boundary ports.

I see. In that scenario it may be desirable to allow for boundary ports with unspecified connector type. Currently, when the user creates an interface port and sets the port-type to a CXInterface, it automatically sets the connector type to CORBA4CCM by default, but the user can set it to null by clicking on the red [X]. Perhaps we should allow this case in general? i.e. any internal port connected to a boundary port with null connector type?

What would a boundary-to-boundary connector be? Since you state that you "do not refer to the instance of 'slavesStateControlFacet' in the top-level AMI4CCM_asm assembly as a boundary port", would a boundary-to-boundary connector only exist in a single assembly diagram where you connect one boundary (delegate) to another boundary (delegate) on the same assembly? That wouldn't make sense as we would just connect the internal ports themselves together or just model it differently.

Right, the case I had in mind was indeed a corner case, where we would have what we called in another similar tool a "pass-through" connector, a connector that connects directly a boundary port in an assembly to another boundary port in the same assembly. It is rather useless for the final, generated artifacts, but it could be useful in early stages of modelling, where you don't yet have an internal part where the boundary ports are supposed to be connected, but will later on be replaced by proper delegation connectors. So in this case the question is whether we flag this boundary-to-boundary as illegal or just ignore it?

So taking into account your input, the following table should describe the rules:

Adding D for "Don't care" to the legend

IRA IRC IFA IFC BRA BRC BFA BFC
IRA N N N Y Y D N D
IRC N N Y D Y N N
IFA N N N N N N
IFC N N N N Y
BRA ? N N N
BRC ? N N
BFA ? N
BFC ?

which can be summarized by the following rules:

The only valid connections are between:

a) internal AMI4CCM receptacle and internal CORBA4CCM facet b) internal CORBA4CCM receptacle and internal CORBA4CCM facet c) internal AMI4CCM or CORBA4CCM receptacle and boundary AMI4CCM, CORBA4CCM or null receptacle d) internal CORBA4CCM facet and boundary CORBA4CCM facet

Ignore: boundary-to-boundary?

Are these rules correct?

emammoser commented 1 year ago

I think the only part that I don't like is the allowed null port. I really don't like the idea of users creating boundary ports that are null and the model doesn't through validation errors at them.

I think I would explode your valid connection "c" into:

c) internal AMI4CCM or CORBA4CCM receptacle and boundary AMI4CCM, CORBA4CCM or null receptacle

c-1) internal AMI4CCM receptacle and boundary AMI4CCM receptacle c-2) internal CORBA4CCM receptacle and boundary CORBA4CCM receptacle

I am also okay to the tool default to CORBA4CCM for the connector type as that is 99% of the time what our users use.

eposse commented 1 year ago

Ok, we won't allow ports with null connector types, but your earlier comment about 9 and 10 suggested that we could also allow

is that not the case or did I misinterpret your earlier comments?

emammoser commented 1 year ago

At the end of the day, the only thing that matters is that some component's internal receptacle eventually connects (or doesn't) to another component's internal facet. The boundary ports and assemblies are just abstractions to help make everything easier to organize and model. Lets just assume that the two bullets that you mentioned above are not allowed.

emammoser commented 1 year ago

I downloaded the RPM from the actions tab for this ticket and found a few issues:

  1. Any delegated port (boundary port) on an Assembly throws an error "An AXCIOMA connector's end ports must be an AMI4CCM Receptacle or CORBA4CCM receptacle or facet and a CORBA4CCM facet if neither end is a order port, or if...." A few issues with this. One, this occurs for DDS ports or any ports on an assembly. We have a ton of different types of ports in our models that are provided by your team (DDS, PSDD) as well as custom ones (PSAT, IO, AI, etc..) and none of these are AMI4CCM or CORBA4CCM. Additionally, I know that in your terminology "border ports" is how your team refers to assembly ports, but that will be very confusing to everyone at NGC as we only refer to them as delegated ports.

  2. I am not sure if this is new, but I when recreating the issue above, I noticed that I couldn't create a new AMI4CCM port on a newly created interface unless I marked the interface itself as "isAsynchronous". I think this is wrong and should be removed. An interface is just an interface specifying operations. It should not require a designation for "isAsynchronous". Only the receptacle should matter if its asynchronous or not.

eposse commented 1 year ago

A couple of questions:

  1. Any delegated port (boundary port) on an Assembly throws an error "An AXCIOMA connector's end ports must be an AMI4CCM Receptacle or CORBA4CCM receptacle or facet and a CORBA4CCM facet if neither end is a order port, or if...."

I assume you mean the validation error occurs on connectors for ports that have port-type other than ports with AMI4CCM or CORBA4CCM connector types, correct?

A few issues with this. One, this occurs for DDS ports or any ports on an assembly. We have a ton of different types of ports in our models that are provided by your team (DDS, PSDD) as well as custom ones (PSAT, IO, AI, etc..) and none of these are AMI4CCM or CORBA4CCM.

I have reproduced it with a port with port-type DDS_Listen. I assume that for ports other than the ones we have been discussing (AMI4CCM/CORBA4CCM) we should fall back to the old validation rules, correct?

Additionally, I know that in your terminology "border ports" is how your team refers to assembly ports, but that will be very confusing to everyone at NGC as we only refer to them as delegated ports.

Right, I'll update the error messages to use "delegated ports" instead.

  1. I am not sure if this is new, but I when recreating the issue above, I noticed that I couldn't create a new AMI4CCM port on a newly created interface unless I marked the interface itself as "isAsynchronous". I think this is wrong and should be removed. An interface is just an interface specifying operations. It should not require a designation for "isAsynchronous". Only the receptacle should matter if its asynchronous or not.

I'm not sure about the workflow in question. I see two possible workflows:

  1. Create a new port (component or delegated) and in the Properties view, select the Port Type to be a CX Interface (such as StateControl_obj in the example provided). This will automatically add a Connector Type field set to CORBA4CCM_Connector by default, but editing this field allows you to select AMI4CCM_Connector, in which case, it will set isAsynchronous to true automatically (with the caveat that there is a refresh issue in the field in the Profile tab and the user needs to click else-where for the refresh into kick-in; this was mentioned in issue #356 where it was stated that a separate ticket would be created, but I can't find any).
  2. Create a new port and in the Properties view, select the Port Type to be AMI4CCM_Port_Type. In this case no Connector Type field is created, and the isAsynchronous field is not set to true automatically (and the port decoration doesn't change).

Are you talking about case 2?

emammoser commented 1 year ago

I have reproduced it with a port with port-type DDS_Listen. I assume that for ports other than the ones we have been discussing (AMI4CCM/CORBA4CCM) we should fall back to the old validation rules, correct?

Yes, exactly. If the port type is not explicitely AMI4CCM and CORBA4CCM, nothing we have been talking about should affect them

Right, I'll update the error messages to use "delegated ports" instead.

Great, thank you!

Are you talking about case 2? Also, I'm not sure what you mean by marking the interface itself as asynchronous. Where are you marking an interface as asynchronous?

Neither, I am talking about defining a new interface altogether (Like StateControl_obj). Before creating any ports on any components, I need to first define an interface somewhere with some number of operations. That interface is then what is used to type a port. I received an error requesting that I needed to change the interface itself to be "isAsynchronous"

eposse commented 1 year ago

I have reproduced it with a port with port-type DDS_Listen. I assume that for ports other than the ones we have been discussing (AMI4CCM/CORBA4CCM) we should fall back to the old validation rules, correct?

Yes, exactly. If the port type is not explicitely AMI4CCM and CORBA4CCM, nothing we have been talking about should affect them

Right, I'll update the error messages to use "delegated ports" instead.

Great, thank you!

Are you talking about case 2? Also, I'm not sure what you mean by marking the interface itself as asynchronous. Where are you marking an interface as asynchronous?

Neither, I am talking about defining a new interface altogether (Like StateControl_obj). Before creating any ports on any components, I need to first define an interface somewhere with some number of operations. That interface is then what is used to type a port. I received an error requesting that I needed to change the interface itself to be "isAsynchronous"

Ok, I see, you need to explicitly set isAsynchronous in the Profile Tab of the interface. What I see is that I can create a port with that interface and set its Connector Type to AMI4CCM_Connector but it will fail to change isAsynchronous to true in this case. Is this what you are observing?

emammoser commented 1 year ago

I believe so. The issue is that an interface can be used 100s of different places, CORBA4CCM or AMI4CCM. However, it appears that the interface needs to be marked as "capable of being AMI4CCM" in order to generate the right tags in IDL. Can we instead add a checkbox to the CX tab that says "AMI4CCM capable" or something like that and have it defaulted to not checked? If checked, then the developer can type ports as AMI4CCM with it. If not, then it can't. Does that make sense?

eposse commented 1 year ago

We can, but I would suggest making that a separate issue as that is not the validation issue here. Does that sound ok to you?

emammoser commented 1 year ago

Yeah, as long as that new warning on the interface itself isn't a new warning (I can't remember), then you can ignore that for now.

eposse commented 1 year ago

Another question: is it valid to connect: a) an AMI4CCM port to a non-CORBA4CCM port? b) a CORBA4CCM port to a non-CORBA4CCM port?

emammoser commented 1 year ago

Another question: is it valid to connect: a) an AMI4CCM port to a non-CORBA4CCM port? b) a CORBA4CCM port to a non-CORBA4CCM port?

No, I don't think it would be.

emammoser commented 1 year ago

I rebuilt the branch with the Actions tab and I am still seeing the same behavior. I believe you have access to SNA's example projects. Have you tried validating our SignalProcessingDataModel example model? In addition to the reproducer that you created earlier in this issue, the SignalProcessingDataModel example model should validate as well. It does not have any AMI4CCM ports.

eposse commented 1 year ago

Sorry, I don't think I have access to that one. I can't find it in neither the cx-reproducers repo nor the source repo. Perhaps you could upload it to the cx-reproducers repo? Thanks.

emammoser commented 1 year ago

Sure, I just pushed it up. It will be under the examples folder

eposse commented 1 year ago

Thanks

eposse commented 1 year ago

The model contains references to the SNA library but I can't find it anywhere either. I've cound the CCM_SPDM library, but not the SNA library. Would it be possible to post that one too? Thanks.

emammoser commented 1 year ago

Pushed up into the ModelLlibraries folder

eposse commented 1 year ago

Thanks

eposse commented 1 year ago

A couple of things:

emammoser commented 1 year ago

Nope, I was grabbing action #326. I have never used actions in github before. It was my (apparently erroneous) assumption that I could rebuild it and it would build the most current commits on that branch. I guess it does not? I will try out 327 and report back

eposse commented 1 year ago

No, it doesn't necessarily build the latest commit. If you want to (re)build something from the Actions page, you can do two things:

1) Click on the "Run-workflow" button on the top-right, select a branch and then click "Run workflow". Note that if you select "papyrus" as the branch, it will build only that, not the latest commit. Typically we name branches in pull-requests following the convention "papyrus_issueNNN", so that's the one to use.

or

2) Click on a particular build job in the list (e.g. the latest) and then click the "Re-run all jobs" button on the top-right.

emammoser commented 1 year ago

Okay, I see that the SPDM model no longer has any validation issues. But the AMI4CCM2 reproducer example still does. Do you see that on your end?

eposse commented 1 year ago

Yes, I see now three errors instead of four. I must have missed them while trying different cases on the model.

eposse commented 1 year ago

Got it. It was a typo in the validation rule. The updated build should be ready in about 20 minutes.

eposse commented 1 year ago

The link to the latest build is here.

Paul has reviewed the code. Let me know if it works for you.

eposse commented 1 year ago

FYI, I opened Issue #465 to deal with the Interface "isAsynchronous"/"AMI4CCM-capable" issue. Please take a look to check if I expressed it correctly. I have a few questions on that one.

emammoser commented 1 year ago

FYI, I opened Issue https://github.com/ZeligsoftDev/CX4CBDDS/issues/465 to deal with the Interface "isAsynchronous"/"AMI4CCM-capable" issue. Please take a look to check if I expressed it correctly. I have a few questions on that one.

Yes, that looks correct to me. Thank you

eposse commented 1 year ago

And were you able to test successfully the latest build for this one? Let me know if we can merge it.

emammoser commented 1 year ago

Not yet, I am working on that now. I will let you know shortly

emammoser commented 1 year ago

I have verified the fix looks good to me. We would also like this change cherry-picked to the 2.3.0 and 2.4.0 maintenance branch/streams

eposse commented 1 year ago

Ok, will do. Thanks. And by the way, I assume you'd like me to work on the new #466 issue next, correct?

emammoser commented 1 year ago

Yes, that will also need to be cherry-picked to 2.3.0 and 2.4.0. I will continue to see if I can find a reproducer for it that we can send you.

eposse commented 1 year ago

Merged to the papyrus branch as well as the 2.3 and 2.4 maintenance branches.