Fixes btc-ag/service-idl#49 #101

huttenlocher · 2018-06-19T12:42:09Z

Fixes #49 : FailableHandlers for .NET

This change is

sigiesec · 2018-06-19T13:27:59Z

After #102 is merged into master, could you please rebase & reformat all touched files and squash the changes with the existing commits?

sigiesec · 2018-06-19T13:29:34Z

com.btc.serviceidl.tests/src/com/btc/serviceidl/tests/cmdline/CommandLineRunnerTest.xtend

@@ -115,7 +115,7 @@ class CommandLineRunnerTest
                "cpp/modules/BTC/Commons/Core/Protobuf/include/btc_commons_core_protobuf_export.h",
                "cpp/modules/BTC/Commons/Core/Protobuf/source/Dependencies.cpp",
                "dotnet/BTC/Commons/Core/Common/BTC.Commons.Core.Common.csproj",
-                "dotnet/BTC/Commons/Core/Common/Properties/AssemblyInfo.cs", "dotnet/BTC/Commons/Core/Common/Types.cs",
+                "dotnet/BTC/Commons/Core/Common/Properties/AssemblyInfo.cs", "dotnet/BTC/Commons/Core/Common/ServiceFaultHandling.cs", "dotnet/BTC/Commons/Core/Common/Types.cs",


Almost the same applies as for the service API below. This must be placed in a different project.

However, I am somewhat confused that a ServiceFaultHandling exists independent from an interface. What should be done here? Maybe when I review the generator itself, this will become clearer or trigger more specific questions.

I think this file will not be used. Do not generate it in this case.

This file will be generated as part of the Protobuf project (see below). It is necessary and is used in the Codec to correctly map the exception for "failed" elements in the failable sequence.

Hm, I thought that still the interface/operation specifies which service faults can be generated, and therefore the service fault handler of the interface is used.

I am not completely sure what makes more sense. However, if it it done the same way in C++ and Java at the moment, it must be done the same way here.

Changing this is beyond the scope of this PR, but we should discuss this at some point.

I opened #105 for this question.

Sorry I get back to this question: Is the file really used in this case? I think there is no failable type in this test case.

That's true, it's not required in this case. Unfortunately, in all languages at the moment the codec is generated in a way that it handles all involved types (e.g. user-defined IDL structs) as well as all "generic" cases (even if not used). So the Protobuf project would not built, if the file is simply removed, since the static function "resolveError" provided by this class is referenced from there in the generic encode/decode functions.

Ok, I see it isn't that easy to change it. So let's leave it this way in the scope of this PR. I opened #110 to track this further.

OK, thanks.

sigiesec · 2018-06-19T13:31:27Z

com.btc.serviceidl.tests/src/com/btc/serviceidl/tests/cmdline/CommandLineRunnerTest.xtend

@@ -225,7 +224,7 @@ class CommandLineRunnerTest
                "dotnet/BTC/Commons/Core/ServerRunner/packages.config",
                "dotnet/BTC/Commons/Core/ServerRunner/paket.references",
                "dotnet/BTC/Commons/Core/ServiceAPI/BTC.Commons.Core.ServiceAPI.csproj",
-                "dotnet/BTC/Commons/Core/ServiceAPI/FooConst.cs", "dotnet/BTC/Commons/Core/ServiceAPI/IFoo.cs",
+                "dotnet/BTC/Commons/Core/ServiceAPI/FooConst.cs", "dotnet/BTC/Commons/Core/ServiceAPI/FooServiceFaultHandling.cs", "dotnet/BTC/Commons/Core/ServiceAPI/IFoo.cs",


API is not the right place for *ServiceFaultHandling.cs. The API should be not depend on the remote protocol. The Protobuf project is probably a better place for this, as it also defines the rest of the protocol.

Probably yes. The initial idea was to share this common functionality for both proxy and dispatcher, and I considered ServiceAPI to be the proper "central" project for it. But in fact, it better belongs to Protobuf, since it is also used by both proxy and dispatcher. I will change it.

Yes,and even though the service fault handling doesn't depend on protobuf right now, it probably will when support for custom exception attributes is fully implemented.

sigiesec · 2018-06-19T13:34:53Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/AppConfigGenerator.xtend

-               <object id="BTC.CAB.ServiceComm.NET.SingleQueue.API.ConnectionFactory" type="«typeResolver.resolve("BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ.NetMQ.NetMqConnectionFactory").fullyQualifiedName», BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ">
-                 <constructor-arg index="0" expression="T(BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ.NetMQ.NetMqConnectionFactory, BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ.NetMQ).DefaultServerConnectionOptions" />
-                 <constructor-arg index="1" ref="BTC.CAB.Logging.API.NET.LoggerFactory" />
+               <object id="BTC.CAB.ServiceComm.NET.SingleQueue.API.ConnectionFactory" type="«typeResolver.resolve("BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ.NetMQ.NetMqConnectionFactory").fullyQualifiedName», BTC.CAB.ServiceComm.NET.SingleQueue.ZeroMQ.NetMQ">


I think the changes to the AppConfigGenerator are not related to the added support for failable, right? Which problem do they solve? Could you split them into a separate commit with an expressive commit message?

True, it is not directly related to the failable handling implementation, but was necessary to be able to use the server runner for testing (I think, the namespace has slightly changed in the current ServiceComm.NET).

Ok, so this is left over from my updating the ServiceComm.NET dependency. I checked whether everything compiles, but the Spring configs are not checked and nothing is executed by the integration tests at the moment. In addition, i originally didn't intend to update the dependency at all, but assumed that ServiceComm.NET was already used,but only noticed that this was not true when I was half done.

sigiesec · 2018-06-19T13:36:35Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/BasicCSharpSourceGenerator.xtend

-      '''«resolve("System.Collections.Generic.IEnumerable")»<«toText(element.type, element)»>'''
+      val isFailable = element.failable
+      val basicType = resolve(element.type)
+      val effectiveType = '''«IF isFailable»«resolveFailableType(basicType.fullyQualifiedName)»«ELSE»«basicType»«ENDIF»'''


This would be somewhat more readable if this did not use a template expression but simple a conditional Xtend expression:
val effectiveType = if (isFailable) resolve... else basicType

True. Will rewrite it this way.

sigiesec · 2018-06-19T13:39:42Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/DotNetFrameworkVersion.java

+       case NET40:
+          return "4.0";
+       case NET45:
+          return "4.5";


The literal value generated before was v4.5.2, for which no enum value exists at all (and it is not equivalent to v4.5 AFAIK).

I am not sure if support for older target frameworks is necessary at all.

However, currently all values of this Enum except for NET46 are unused. It is not possible to select them e.g. via a command line parameter. Unless this is changed, they can and should be removed.

Yes, the best thing is to remove unused values. These are just remnants from the past.

sigiesec · 2018-06-19T14:24:35Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/TypeResolver.xtend

+    def String resolveFailableProtobufType(EObject element, EObject container)
+    {
+        val namespace = GeneratorUtil.getTransformedModuleName
+        (


Please reformat this with the Xtend formatter.

sigiesec · 2018-06-19T14:25:00Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/TypeResolver.xtend

+        val namespace = GeneratorUtil.getTransformedModuleName
+        (
+            ParameterBundle
+            .createBuilder(com.btc.serviceidl.util.Util.getModuleStack(com.btc.serviceidl.util.Util.getScopeDeterminant(container)))


Use extension syntax

sigiesec · 2018-06-19T14:25:29Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/TypeResolver.xtend

@@ -30,6 +30,7 @@ import org.eclipse.xtext.naming.IQualifiedNameProvider
 import org.eclipse.xtext.naming.QualifiedName

 import static extension com.btc.serviceidl.generator.common.Extensions.*
+import com.btc.serviceidl.idl.AliasDeclaration


Broken order of imports.

sigiesec · 2018-06-19T14:27:07Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/ServiceFaultHandlingGenerator.xtend

+import com.btc.serviceidl.util.Constants
+
+@Accessors(NONE)
+class ServiceFaultHandlingGenerator extends GeneratorBase


If I saw it correctly, this does not depend on ServiceComm explicitly. I think it makes some assumptions made by ServiceComm, but it might be used in some similar context as well. This should be documented.

sigiesec · 2018-06-19T14:28:46Z

com.btc.serviceidl/src/com/btc/serviceidl/generator/dotnet/ProxyDispatcherGeneratorBase.xtend

    {
        val is_sequence = com.btc.serviceidl.util.Util.isSequenceType(type)
        if (is_sequence)
        {
            val ultimateType = com.btc.serviceidl.util.Util.getUltimateType(type)
-            if (ultimateType instanceof PrimitiveType && (ultimateType as PrimitiveType).integerType !== null)
+            val isFailable = com.btc.serviceidl.util.Util.isFailable(type)
+            if (isFailable)


Use extension syntax and inline the isFailable variable into the single location where it is used

…omm.NET 0.6

…yProtocol no longer generated; target .NET Framework version changed to 4.6; reformatted using project's Xtend formatter

huttenlocher requested a review from a team June 19, 2018 12:42

sigiesec requested changes Jun 19, 2018

View reviewed changes

sigiesec mentioned this pull request Jun 20, 2018

Suppress generation of unnecessary ServiceFaultHandling.cs files #110

Open

huttenlocher force-pushed the issue-49 branch from a9365db to 5f713a1 Compare June 21, 2018 11:20

sigiesec and others added 4 commits June 21, 2018 15:19

Fix AppConfigGenerator to generate correct configuration for ServiceC…

5ea1508

…omm.NET 0.6

Fixes #49; improvements after review; unused files ProxyData and Prox…

6a20701

…yProtocol no longer generated; target .NET Framework version changed to 4.6; reformatted using project's Xtend formatter

Show more exception details

f92db02

Fixes output configuration when using generator from Eclipse Runtime

2651bfa

sigiesec force-pushed the issue-49 branch from 5f713a1 to 2651bfa Compare June 21, 2018 13:19

sigiesec approved these changes Jun 21, 2018

View reviewed changes

sigiesec merged commit d0abd47 into master Jun 21, 2018

sigiesec deleted the issue-49 branch June 21, 2018 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes btc-ag/service-idl#49 #101

Fixes btc-ag/service-idl#49 #101

huttenlocher commented Jun 19, 2018 •

edited by sigiesec

Loading

sigiesec commented Jun 19, 2018

sigiesec Jun 19, 2018

sigiesec Jun 20, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 20, 2018

sigiesec Jun 20, 2018

sigiesec Jun 20, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 20, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 20, 2018

huttenlocher Jun 21, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 20, 2018

sigiesec Jun 19, 2018

sigiesec Jun 19, 2018

huttenlocher Jun 21, 2018

Fixes btc-ag/service-idl#49 #101

Fixes btc-ag/service-idl#49 #101

Conversation

huttenlocher commented Jun 19, 2018 • edited by sigiesec Loading

sigiesec commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huttenlocher commented Jun 19, 2018 •

edited by sigiesec

Loading