Complex Mapping feature (LIGHTLY TESTED, NEEDS REVIEW) by metasemi · Pull Request #31 · adobe-apiplatform/user-sync.py

metasemi · 2017-02-28T18:03:12Z

No description provided.

…feature - config support only; functionality to come.

…pt 1.

…pt 2.

…med. (Still not used though.)

… confirmed.

…d - will test with hook invoke.

…complete smoke test working.

phil-levy · 2017-02-28T19:19:37Z

+            for extension_config in extensions_config.iter_dict_configs():
+                context = extension_config.get_string('context')
+                if context == 'per-user':
+                    if (after_mapping_hook == None):


Is this test needed/correct? Looks like after_mapping_hook set to None a few lines up.

This is testing for multiple per-user extensions; see the matching else.

Got it. Missed the loop.

adobeDan

This is almost ready to be merged, and most of my suggested changes are not critical, but a few at least need to be considered carefully before we merge.

adobeDan · 2017-02-28T18:41:04Z

+        after_mapping_hook = None
+        extended_attributes = None
+        extensions_config = self.main_config.get_list_config('extensions', True)
+        if (extensions_config != None):


In line 202 you use the correct construction if group is None but here you are not: it should be if extensions_config is not None. I suspect you are being influenced by the existing code style (see line 307), but this code is materially worse in performance because it forces a type check.

I used the equality when in the midst of a block of code that was already using it; otherwise I did what you're calling the right way. Should be addressed globally.

adobeDan · 2017-02-28T18:43:15Z

-                    organization_name = user_sync.rules.OWNING_ORGANIZATION_NAME
-                if (len(group_name) == 0):
+                group = self.create_dashboard_group(dashboard_group)
+                if (group is None):


Ouch! Parentheses around if conditions are not only unnecessary in Python, they are actively discouraged. I assume you are doing this because the rest of the Ensemble code does this, but yuck! We really should fix all this code. Parentheses in Python should be used entirely for precedence and readability; nothing else.

Yes, following the style of the codebase...

adobeDan · 2017-02-28T18:54:46Z

+                if context == 'per-user':
+                    if (after_mapping_hook == None):
+                        after_mapping_hook_text = extension_config.get_string('after_mapping_hook')
+                        if (after_mapping_hook_text is not None):


This kind of deeply nested if (the three tests including this one) where the else parts are errors make the code really hard to follow. You should do the "go" thing here: invert the test and have the if part log the warning and skip the rest of the code.

if context != 'per-user': self.logger.warning("Unrecognized extension context '%s' ignored", context) continue ...

Also consider breaking this loop or body of this loop out as a helper function.

Leaving this for now, but agree with you. Will fix if I have time.

adobeDan · 2017-02-28T19:00:58Z

+                            # thoroughly tested.
+                            #
+                            for extended_dashboard_group in extension_config.get_list('extended_dashboard_groups'):
+                                group = self.create_dashboard_group(extended_dashboard_group)


I can see from the code later on that create_dashboard_group ignores the group if it already exists as a dashboard group. This means you are guarding against people specifying groups in their extended_dashboard_groups that are already groups in the mapping, which seems like it would be a common (and forgiveable) mistake. A comment to that effect would be useful here (and also in the docs).

Actually create_dashboard_group did not behave that way. But it does now :)

adobeDan · 2017-02-28T19:08:07Z

    :rtype (bool, iterable(dict))
    '''
+    if (extended_attributes is not None):
+        self.logger.warning("CSV directory connector doesn't support extended_attributes; ignored")


Really? Seems like it would be really really easy for the CSV connector to support extended attributes, since you could just add them as column headers. And this seems like it would be a really, really handy way to test extended logic on just a few people...

Doesn't really make sense for csv to have extended attrs because all csv attrs are always included in the data. No need to ask it to include more.

Right, that's exactly my point 👍 . We are warning the user that the CSV connector doesn't support extended attributes but in fact I believe the CSV connector already supports extended attributes without our doing any coding!

So I think the action here is to silently ignore the extended attrs list. Just like with AD, if there is no such attr, it just won't show up.

Right, so Phil and I agree: just remove this test. The CSV directory connector does support extended attributes, as far as we know. Have you verified otherwise, Mike?

Great, done.

adobeDan · 2017-02-28T19:46:46Z

+                self.after_mapping_hook_scope['target_attributes'].pop('source_attributes', None)
+
+                # invoke the customer's hook code
+                # [TODO morr 2017-02-27: Not putting any guardrails around the customer's code, which is treated as


You're giving them a confined scope; there's nothing really they can do except crash us or hang us. And there's no good way to guard against that :).

What about exceptions flying out?

Right, that's the crash case. I don't think we should guard against that, because we can't continue, and it's pointless for us to catch, log, and rethrow.

Agreed, removing the comment. Similarly we don't catch errors compiling the hook code, and the resulting behavior is excellent.

adobeDan · 2017-02-28T19:50:16Z

+                else:
+                    self.logger.error('Target dashboard group %s is not known; ignored', target_group_qualified_name)
+
+        self.logger.info('Total directory users after filtering: %d', len(filtered_directory_user_by_user_key))


Is there some corresponding log of Total directory users before filtering? If not, why not, and if so, why isn't this statement next to it?

This was not changed by me, but fwiw this is not confusing in the actual log, modulo the fact that the term "filtering" is used in multiple ways - the --users way and the all_users_filter way. (This is talking about the former.) Anyway I'm not doing anything here.

adobeDan · 2017-02-28T19:56:03Z

-
-
+
+    def log_after_mapping_hook_scope(self, before_call):


Ouch! A boolean variable named before_call, so the call has True or False in it? That's really opaque! Either use keyword variables before_call=False, after_call=False and have the caller specify one of them (and err if caller doesn't), or have a variable context which has a string value which must be before_call or after_call (and is an error otherwise).

Agree with your complaint, though I can't say I'm crazy about either proposed fix. Leaving alone for now.

adobeDan · 2017-02-28T19:57:04Z

+                # invoke the customer's hook code
+                # [TODO morr 2017-02-27: Not putting any guardrails around the customer's code, which is treated as
+                # friendly. Is there anything we should be doing?
+                self.log_after_mapping_hook_scope(True)


Ouch! How opaque is this! See comment at line 683.

Leaving alone for now.

adobeDan · 2017-02-28T19:58:44Z

+    def log_after_mapping_hook_scope(self, before_call):
+        when = 'before' if before_call else 'after'
+        if (before_call):
+            self.logger.debug('.')


what is this about? are you trying to get a separator line in the log? I bet we could be a lot clearer than a simple period.

It's pretty great the way it is. What would you suggest? :)

…view meeting.

Mike Orr added 16 commits February 24, 2017 11:40

Support 'extensions' section in main config file for Complex Mapping …

b1523a3

…feature - config support only; functionality to come.

Load extended user attributes as specified in config; unproven, attem…

3636110

…pt 1.

Load extended user attributes as specified in config; unproven, attem…

10e79d4

…pt 2.

(Empty commit) Load extended user attributes; correct behavior confir…

25b55d9

…med. (Still not used though.)

Prepare hook call (not doing it yet); unproven, attempt 1.

30e1091

Prepare hook call (not doing it yet); unproven, attempt 2.

b2049ab

Prepare hook call (not doint it yet); unproven, attempt 3.

3898e65

(empty commit) Prepare hook call (not doing it yet); correct behavior…

db8337b

… confirmed.

Read and init extended dashboard groups, compile hook code; not teste…

ae39428

…d - will test with hook invoke.

Execute hook call; unproven, attempt 1.

f8eb337

Execute hook call; unproven, attempt 2.

e72fb9e

Execute hook call; unproven, attempt 3.

afa7a03

Execute hook call; unproven, attempt 4.

6182f88

Execute hook call; unproven, attempt 5.

8adc563

(Empty commit) Execute hook call; correct behavior confirmed, simple …

7219557

…complete smoke test working.

Added a comment.

06fd491

adobeDan requested review from adobeDan and phil-levy February 28, 2017 18:35

phil-levy reviewed Feb 28, 2017

View reviewed changes

adobeDan suggested changes Feb 28, 2017

View reviewed changes

Mike Orr added 2 commits February 28, 2017 14:14

Fixes to address offline review comments; checking in untested for re…

1bb3541

…view meeting.

Incorporate second round of review feedback; still not tested.

9c603a9

metasemi requested review from adobeDan and phil-levy March 1, 2017 00:46

phil-levy merged commit b0fa845 into adobe-apiplatform:master Mar 1, 2017

phil-levy mentioned this pull request Mar 1, 2017

Revert "Complex Mapping feature (LIGHTLY TESTED, NEEDS REVIEW)" #32

Merged

Uh oh!

Conversation

metasemi commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adobeDan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants