[GAIAPLAT-672] Implement connect/disconnect translation in gaiat #841

simone-gaia · 2021-08-17T20:37:36Z

Add the connect/disconnect handling logic in gaiat.

Add declarative_connect_disconnect_handler_t in gaiat + relative matchers.
Changed disconnect() semantic for 1:1 relationship to make it possible only in the form table.link.disconnect(). Link name must be specified and no arguments required.
Create GaiaTable attribute to add to the classes that represent tables in the DB.
Delete err_invalid_table_name message because it is a duplicate of err_table_not_found.
Add link_data_t to table_data_t in table_navigation.h
Disable .clang-tidy variables casing warnings for clang code.

This reverts commit 49bbe9a.

…into rondelli-conn-disconn-gaiat-2

…-conn-disconn-gaiat-2

…into rondelli-conn-disconn-gaiat-2

simone-gaia · 2021-08-17T20:38:12Z

production/tools/gaia_translate/src/main.cpp

@@ -3025,6 +3028,140 @@ class declarative_break_continue_handler_t : public MatchFinder::MatchCallback
    Rewriter& m_rewriter;
 };

+// AST handler that is called when declarative break or continue are used in the rule.


third_party/production/TranslationEngineLLVM/clang/.clang-tidy

fineg74 · 2021-08-17T23:52:39Z

production/tools/gaia_translate/src/main.cpp

+            {
+                // Changes connect() to insert() for 1:N relationships.
+                // TODO https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1181
+                m_rewriter.ReplaceText(method_call_expr->getExprLoc(), c_connect_length, "insert");


Do you have full EDC support for this and remove ? I.e. the spec says it should handle the cases when records already connected etc.

Good point. This is supported in the 1:1 connect disconnect (at EDC level) but not for 1:n. I would implement this in EDC to make the 2 APIs somewhat symmetric.

Created a task for it: https://gaiaplatform.atlassian.net/browse/GAIAPLAT-1193

fineg74 · 2021-08-17T23:53:29Z

production/tools/gaia_translate/src/main.cpp

+                    method_call_expr->getExprLoc().getLocWithOffset(c_disconnect_length + 1),
+                    method_call_expr->getEndLoc());
+
+                param_loc.dump(*result.SourceManager);


You don't need this

Damn, forgot it.

LaurentiuCristofor

I have no special observations. The logic should be reviewed by Greg.

daxhaw · 2021-08-18T07:48:48Z

production/tools/gaia_translate/src/main.cpp

+        auto src_table_iter = table_data.find(src_table_name);
+        if (src_table_iter == table_data.end())
+        {
+            cerr << "Table '" << src_table_name << "' was not found in the catalog." << endl;


I'd like to check in my changes before yours and then you can pick up the error strings. Ideally there is one you can use like err_table_not_found.

I've pushed to master. Thanks!

daxhaw · 2021-08-18T07:57:18Z

production/tools/gaia_translate/src/main.cpp

@@ -56,6 +56,9 @@ bool g_is_rule_prolog_specified = false;
 constexpr int c_encoding_shift = 16;
 constexpr int c_encoding_mask = 0xFFFF;

+constexpr size_t c_connect_length = 7;


nit: maybe c_connect_keyword_length would be more descriptive.

daxhaw · 2021-08-18T07:59:09Z

production/tools/gaia_translate/src/main.cpp

@@ -3025,6 +3028,140 @@ class declarative_break_continue_handler_t : public MatchFinder::MatchCallback
    Rewriter& m_rewriter;
 };

+// AST handler that is called when declarative break or continue are used in the rule.
+class declarative_connect_disconnect_handler_t : public MatchFinder::MatchCallback


Does it make sense to break this up into two handlers? (connect handler and disconnect handler)?

That's what I started with until I realized that the code is 95% the same. An alternative would be to have a parent class that handles all the common logic and subclasses with specializations.

I can go down this route if you wish.

Well, parent class or a helper function that both call. Just then you wouldn't have to check which keyword it is "connect" or "disconnect" and the two cases are different. I don't mind having single-purpose matchers.

Actually, when we are going to add connect/disconnect to the EDC containers the code will be exactly the same.

daxhaw · 2021-08-18T08:00:09Z

production/tools/gaia_translate/src/main.cpp

+        bool need_link_field = false;
+
+        const auto* method_call_expr = result.Nodes.getNodeAs<CXXMemberCallExpr>("connectDisconnectCall");
+        bool is_connect = method_call_expr->getMethodDecl()->getName().str() == "connect";


Someday I'd like to consolidate all our declarative language keywords into a single string table... (not saying you should do this now, but maybe move the string to a c_connect_keyword constant.

but maybe move the string to a c_connect_keyword constant.

Done.

daxhaw · 2021-08-18T08:05:44Z

production/tools/gaia_translate/src/main.cpp

+
+        if (link_name.empty())
+        {
+            cerr << "Impossible to find a link between table " << src_table_name << " and table " << dest_table_name << endl;


consider rewording from "Impossible to ..." to something like "A link does not exist between ...". Then add to DiagnosticSemaKinds.td (in alphabetical order) where our Gaia errors are at the bottom of the file grouped by err, note, and warn. Then use the gaiat::diag().emit(...) construct to report an LLVM error. Of course, these changes will only be visible after I merge PR-820.

consider rewording from "Impossible to ..." to something like "A link does not exist between ...". T

Done.

WIll do the rest after you merge your PR.

production/tools/gaia_translate/src/main.cpp

production/tools/gaia_translate/tests/test_connect_disconnect.cpp

daxhaw · 2021-08-18T08:14:21Z

production/tools/gaia_translate/tests/test_connect_disconnect.cpp

+
+TEST_F(test_connect_disconnect, test_connect_1_n)
+{
+    gaia::rules::initialize_rules_engine();


Looks like these two lines (init rules engine and unsubscribe) are used in every test. Consider moving to Setup.

Done. @waynelwarren can we do the same for other tests?

Yes, I can take a look at this.

daxhaw · 2021-08-18T08:16:30Z

production/tools/gaia_translate/tests/test_connect_disconnect.ruleset

+
+using namespace gaia::barn_storage;
+
+ruleset test_connect_disconnect


probably handled below but why no disconnect rule/test here? I feel misled by the ruleset name :).

This should not be here, removed. We squeeze all the tests into one ruleset, and then we create different tests files for each type of test. This greatly improves compile time.

production/tools/gaia_translate/tests/test_rulesets.ruleset

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp

daxhaw

Mostly nits and a confusion about the integration tests. Cool to see this functionality almost in!

…into rondelli-conn-disconn-gaiat-2

waynelwarren

LGTM. I have built and tested this branch. Added one small test.

* simple new disconnect/remove test * using size() method rather than scanning members

daxhaw · 2021-08-20T17:59:51Z

third_party/production/TranslationEngineLLVM/clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -9526,7 +9526,7 @@ def err_duplicate_field : Error<
  "Duplicate field '%0' found in table '%1'. Qualify your field with the "
  "table name (table.field) to disambiguate field names that occur in more "
  "than one table. You can also restrict the list of tables to search by "
-  "specifing them in the ruleset 'tables' attribute.">;
+  "specifying them in the ruleset 'tables' attribute.">;


Good catch. Thanks!

daxhaw · 2021-08-20T18:00:38Z

third_party/production/TranslationEngineLLVM/clang/include/clang/Basic/DiagnosticSemaKinds.td

@@ -9578,8 +9578,6 @@ def err_invalid_parent_table : Error<
  "Incorrect parent table in the relationship '%0'.">;
 def err_invalid_table_field : Error<
  "Field '%0' has no table in the catalog.">;
-def err_invalid_table_name : Error<


We don't need this error any more? Ah, I guess this was a dup with err_table_not_found. Another good catch.

Yep. This is subsumed by err_table_not_found.

daxhaw · 2021-08-20T18:03:20Z

third_party/production/TranslationEngineLLVM/clang/test/Parser/gaia_rule_attributes.cpp

@@ -70,7 +70,7 @@ ruleset test38

 ruleset test39
 {
-    on_update(value) // expected-error {{Duplicate field 'value' found in table 'actuator'. Qualify your field with the table name (table.field) to disambiguate field names that occur in more than one table. You can also restrict the list of tables to search by specifing them in the ruleset 'tables' attribute.}}


😭 Thank you for fixing this up for me. Embarrassing.

daxhaw

Looks good.

simone-gaia and others added 12 commits August 11, 2021 11:01

Start gaiat work

147368a

More gaiat stuff

2501a22

Add support for EDC types in connect/disconnect

49bbe9a

Revert "Add support for EDC types in connect/disconnect"

a95f604

This reverts commit 49bbe9a.

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

d938976

…into rondelli-conn-disconn-gaiat-2

A few small changes to test 1:1 relationships

5f13c94

Add support for connect/disconnect in gaiat

ae6fa77

Merge remote-tracking branch 'origin/wayne/test-1-to-1' into rondelli…

b01a501

…-conn-disconn-gaiat-2

More Stuff

4449622

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

449cb8b

…into rondelli-conn-disconn-gaiat-2

Improvements

2b74d12

minor fix

a5ac71a

simone-gaia requested review from waynelwarren, LaurentiuCristofor, daxhaw and fineg74 August 17, 2021 20:37

simone-gaia commented Aug 17, 2021

View reviewed changes

third_party/production/TranslationEngineLLVM/clang/.clang-tidy Show resolved Hide resolved

fineg74 reviewed Aug 17, 2021

View reviewed changes

LaurentiuCristofor reviewed Aug 18, 2021

View reviewed changes

daxhaw reviewed Aug 18, 2021

View reviewed changes

production/tools/gaia_translate/src/main.cpp Outdated Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

production/tools/gaia_translate/tests/test_connect_disconnect.cpp Outdated Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

production/tools/gaia_translate/tests/test_connect_disconnect.cpp Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

production/tools/gaia_translate/tests/test_rulesets.ruleset Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

production/tools/gaia_translate/tests/test_rulesets.ruleset Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

third_party/production/TranslationEngineLLVM/clang/lib/Sema/SemaGaia.cpp Show resolved Hide resolved

daxhaw reviewed Aug 18, 2021

View reviewed changes

simone-gaia added 5 commits August 18, 2021 12:09

Address PR feedback

13556c8

Merge branch 'master' of https://github.com/gaia-platform/GaiaPlatform …

1574a22

…into rondelli-conn-disconn-gaiat-2

Integrate messaging changes

840d5ad

Fix spelling error

920eece

Update error messages

925ff58

waynelwarren approved these changes Aug 20, 2021

View reviewed changes

Simple new disconnect/remove test (#854)

fb25733

* simple new disconnect/remove test * using size() method rather than scanning members

daxhaw reviewed Aug 20, 2021

View reviewed changes

daxhaw approved these changes Aug 20, 2021

View reviewed changes

simone-gaia merged commit 52c350d into master Aug 20, 2021

LaurentiuCristofor deleted the rondelli-conn-disconn-gaiat-2 branch October 15, 2021 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GAIAPLAT-672] Implement connect/disconnect translation in gaiat #841

[GAIAPLAT-672] Implement connect/disconnect translation in gaiat #841

simone-gaia commented Aug 17, 2021 •

edited

Loading

simone-gaia Aug 17, 2021

fineg74 Aug 17, 2021

simone-gaia Aug 18, 2021

fineg74 Aug 17, 2021

simone-gaia Aug 18, 2021

LaurentiuCristofor left a comment

daxhaw Aug 18, 2021 •

edited

Loading

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021

daxhaw Aug 18, 2021

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021 •

edited

Loading

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021 •

edited

Loading

simone-gaia Aug 18, 2021

daxhaw Aug 18, 2021

simone-gaia Aug 18, 2021

waynelwarren Aug 20, 2021

daxhaw Aug 18, 2021

simone-gaia Aug 18, 2021

daxhaw left a comment

waynelwarren left a comment

daxhaw Aug 20, 2021

daxhaw Aug 20, 2021 •

edited

Loading

simone-gaia Aug 20, 2021

daxhaw Aug 20, 2021

daxhaw left a comment


		using namespace gaia::barn_storage;

		ruleset test_connect_disconnect

[GAIAPLAT-672] Implement connect/disconnect translation in gaiat #841

[GAIAPLAT-672] Implement connect/disconnect translation in gaiat #841

Conversation

simone-gaia commented Aug 17, 2021 • edited Loading

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

LaurentiuCristofor left a comment

Choose a reason for hiding this comment

daxhaw Aug 18, 2021 • edited Loading

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

daxhaw Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw Aug 18, 2021 • edited Loading

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

daxhaw left a comment

Choose a reason for hiding this comment

waynelwarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxhaw left a comment

Choose a reason for hiding this comment

simone-gaia commented Aug 17, 2021 •

edited

Loading

daxhaw Aug 18, 2021 •

edited

Loading

daxhaw Aug 18, 2021 •

edited

Loading

daxhaw Aug 18, 2021 •

edited

Loading

daxhaw Aug 20, 2021 •

edited

Loading