iframe-proxy

renaudll · 2017-03-07T18:17:51Z

Hi,

We started using mockgun for testing our pipeline at Squeeze.
However I've realized that some calls to find that worked with Shotgun failed with mockgun.
This was mainly related to :

Using nested multi-entity when filtering (ex: ["project.Project.users.HumanUser.login", "is", "user1"])
Requested fields (indirectly through the order argument) not being returned.

I've made the changes and implemented some additional tests, hope it help!

victoriagrey · 2017-03-07T21:38:30Z

jfboismenu · 2017-03-13T12:18:30Z

Hi @renaudll , I'm dropping by just to let you know that I will be reviewing your fix later this week. We're so happy that people are contributing to mockgun. :) Shouldn't take long.

jfboismenu

Hi,
Thanks for your help. Unfortunately there seems to be a few issues with your pull request which prevents us from merging it as is.

Notably, it seems to add two behavioral changes that are not actually supported by a real Shotgun server, namely

ordered fields are not part of the result set
it appears you can't order on a multi entity field

There's a lot of good in there however and we'd love if you could submit an update that would address these issues.

jfboismenu · 2017-03-15T11:54:42Z

+
+        # Include fields from the order argument in the searched fields
+        if order:
+            for o in order:


You are trying to fix the find query here in order to be able to sort on fields that are not part of the returned result. This is awesome!!! Thanks so much.

However, it seems to be leaking the ordered fields into the result set, which doesn't seem to be the behaviour of the Shotgun API. I've run shotgun.find("HumanUser", [], order=[{'field_name': 'login', 'direction': 'asc'}]) like your test does on my Shotgun server and the field doesn't come back.

Did it on your end? Which version of Shotgun are you using?

Your are right, the fields were leakings! I've pushed a new commit that fix this.

jfboismenu · 2017-03-15T12:01:46Z

+        self.assertEqual(self._pipeline_configutation_2['id'], item['id'])
+
+        # Test descending order
+        item = self._mockgun.find_one("PipelineConfiguration", [], order=[{'field_name': 'users.HumanUser.login', 'direction': 'desc'}])


Could you provide another example of this working on an actual server? Running this query on my Shotgun server yields:

Fault: API read() CRUD ERROR #10: Read failed for entity type [PipelineConfiguration]: Invalid Shotgun Field PipelineConfigurationUserConnection.login

I'm surprised that we could sort on a multi entity field. If there are multiple entities attached to the field which value are we supposed to even use for sorting. I'll check back with the backend gurus.

To be clear, there is nothing wrong with the update to the order code you provided (except for it returning an extra field, as discussed above), so if you could just provide a unit test that would succeed on a real server that would be great. If I am indeed right that we can't order on multi-entity field values then we will probably want at a later date fix Mockgun to prevent it.

We are using a lot of sorting on nested entity field on our side.
For example in production we have a custom entity named Episode that contain Sequences.
Here is the code I use to get all of it's sequence in the correct order.

filters = [ ["sg_sequence.Sequence.sg_episode", "is", {'type': self.type, 'id': self.id}] ] order = [ {'field_name': 'sg_sequence.Sequence.sg_sequence_order', 'direction': 'asc'}, {'field_name': 'sg_cut_order', 'direction': 'asc'}, ]

However I just realised that the sg_episode and sg_sequence are 'entity' fields and the PipelineConfiguration.users is an 'multi_entity' fields. You might be right it may be not possible to sort using 'multi_entity' fields.

I was using the 'PipelineConfigure.users' field to have a simple test case without the need to create custom fields, however I might have to rename 'test_find_order_multi_entity' to 'test_find_order_nested_entity' and design a better test case that exclusively use 'entity' fields instead of 'multi_entity' fields. I'll think of something and update the pull request accordingly!

How about sorting pipeline configurations based on the project's name? :D

Yes good idea, I've pushed an update with this change. In a perfect world I would have loved to test complex nested queries like 'field1.EntityType.field2.EntityType.field3' etc but this should do the trick for now.

jfboismenu · 2017-03-15T12:04:12Z

+        self.assertEqual(self._pipeline_configutation_1['id'], item['id'])
+
+    def test_find_order_fields(self):
+        """Ensure that additional fields passed through the order argument but NOT via the field argument are added to the resulting fields."""


I think what we need here is actually a test that sorts on a nested field, like sorting PipelineConfigurations based on the associated project name.
And then also assert that the field we used to sort is not part of the result.

jfboismenu · 2017-03-15T13:13:52Z

                        values.append(sub_field_value)
                    return values
                # not multi entity, must be entity.
+                elif field_value is None:


Adding these

elif field_value is None: return None

two lines to the original method are all that is necessary for your tests to pass. I recommend you restore the original code.

I'm not sure I'm following, by just cherry-picking those two line in the upstream code the tests "test_find_order_multi_entity" fail with the following key error "KeyError: 'users.HumanUser.login'". The rest of the changes in this method is necessary to properly handle requests like "users.HumanUser.login".

Did you undo the code in find as well? I meant that you should undo the changes from _get_field_from_row method. I've pushed a branch that restores the original code from _get_field_from_row and injects the two lines. You can see the diff with master here.

When running the tests on that branch, everything still passes.

You are also right! I've continued my work from your branch.

test_find_by_sub_entity_field_nested fail on upstream fix test_find_order_fields

jfboismenu · 2017-03-21T15:13:44Z

Hi @renaudll , do you have any updates?

renaudll · 2017-03-22T13:14:57Z

Hi @jfboismenu , sorry for the delay. I want to continue working on this but don't have any free time atm. I will try sending an update next friday. Thanks!

jfboismenu · 2017-03-22T19:11:01Z

Great! Sorry, I didn't mean to sound pushy. We appreciate the time you are taking to fix this.
Looking forward to the update.

find: Fix leaking field from order create: Automatically create_at fields test_mockgun: Sort by entity field instead of multi-entity field test_mockgun: add test_find_order_fields_leak test_mockgun: add test_find_order_date created

renaudll · 2017-03-31T19:06:57Z

Hi @jfboismenu , sorry for the delay, it was a crazy week! I've pushed an update that address all the points listed. Thank you so much for your time reviewing this and sorry again for the slow response.

P.S.: I've took the liberty of adding another little fix that set the 'create_at' field on the create method (SqueezeStudioAnimation@45ce172#diff-443157c398f7038db0d1eb0401aa7d4eR382). Don't hesitate to ask for a separated pull request!

jfboismenu

Only remaining issue is the created_at support you've added. Could you fix it please? Everything else looks great! Thanks for that awesome contribution.

jfboismenu · 2017-03-31T19:42:05Z

+    def test_find_order_fields_leak(self):
+        """Ensure that additional fields passed through the order argument but NOT via the field argument are not added to the resulting fields."""
+        item = self._mockgun.find_one("HumanUser", [], fields=['email'], order=[{'field_name': 'login', 'direction': 'asc'}])
+        self.assertSetEqual(set(item.keys()), set(['id', 'type', 'email']))  # note: we don't verify the order yet


Unfortunately, assertSetEqual was introduced in Python 2.7 and we still need to support 2.6. assertEqual is our only friend for comparison.

jfboismenu · 2017-03-31T19:46:25Z

-        self._update_row(entity_type, row, data)        
+        self._update_row(entity_type, row, data)
        row["id"] = next_id
+        row["created_at"] = datetime.datetime.now()


created_at can be set by a shotgun.create call. You should test for "created_at" not in data first.

jfboismenu · 2017-03-31T19:51:57Z

        ]

-        # handle the ordering of the recordset
+        # Request additional fields from results


I find the wording a bit confusing here, especially with additional. Extract fields from rows seems reasonable to me. Sorry to be nitpicky, but that comment feels confusing to me.

jfboismenu · 2017-03-31T19:53:44Z

+        fields_to_remove = all_fields - requested_fields
+        for v in val:
+            for field in fields_to_remove:
+                v.pop(field)


Ah, never noticed that dict had a pop method just like list. Looks nicer than using del v[field]. You get the result back as well.

Nice!

jfboismenu · 2017-03-31T19:55:30Z


 """

+import copy


This doesn't seem to be used anywhere in the file.

renaudll · 2017-04-19T17:43:34Z

Hi @jfboismenu , sorry again for the delay.

I've done the tweaks, however I still needed to modify mockgun a bit to ensure our pipeline unit-tests are working accordingly. see SqueezeStudioAnimation@7ea4244#diff-443157c398f7038db0d1eb0401aa7d4eL659

I've added tests in test_find_by_sub_entity_field that demonstrate the issue. What do you think?

jfboismenu

One typo and one question. Otherwise this looks good to go.

jfboismenu · 2017-04-27T18:22:03Z

        row["id"] = next_id
-        row["created_at"] = datetime.datetime.now()
+
+        if "create_at" not in row:


Fixed! Sorry about that!

jfboismenu · 2017-04-27T18:22:44Z

                    raise ShotgunError("Invalid deep query field %s.%s" % (entity_type, field))

-                # make sure that types in the query match type in the linked field
-                if entity_type2 != field_value["type"]:


Could you elaborate on why removing this was needed?

You can check the test_find_by_sub_entity_field I've added in my previous commit.
It seem that this error get triggered when filtering multi-entity fields.

A simple example is the 'entity' field on a 'Task'.
The following filters are valid on a shotgun server but will fail on mockgun with this check:
sg.find('Task', [['entity.Asset.code', 'is', 999]])
sg.find('Task', [['entity.Shot.code', 'is', 999]])

I might be able to extend it to work better with multi entities by querying what are the valid entity types. However in the time we had removing it was the "quick and dirty" way. :|

The previous code was incorrect, let's remote it. We're in a better shape now.

jfboismenu · 2017-05-04T19:18:15Z

Thank you so much @renaudll . This was awesome.

This reverts commit 73554b5.

jfboismenu · 2017-05-04T19:26:06Z

fix nested multi-entities query

582b571

victoriagrey requested a review from manneohrstrom March 8, 2017 17:45

jfboismenu suggested changes Mar 15, 2017

View reviewed changes

renaudll and others added 2 commits March 15, 2017 17:30

Fix order fields leaking into requested fields, tweak unittests

d61147b

test_find_by_sub_entity_field_nested fail on upstream fix test_find_order_fields

minimal fix for _get_field_from_row

74ebce4

jfboismenu removed the request for review from manneohrstrom March 22, 2017 19:13

renaudll added 2 commits March 31, 2017 14:53

Fix leaking field, tweak unittests

45ce172

find: Fix leaking field from order create: Automatically create_at fields test_mockgun: Sort by entity field instead of multi-entity field test_mockgun: add test_find_order_fields_leak test_mockgun: add test_find_order_date created

Merge remote-tracking branch 'origin/squeeze_fixes' into squeeze_fixes

c0e3dc8

jfboismenu suggested changes Mar 31, 2017

View reviewed changes

extend test_find_by_sub_entity_field, various other fixes

7ea4244

jfboismenu approved these changes Apr 27, 2017

View reviewed changes

fix typo

52da745

jfboismenu merged commit 73554b5 into shotgunsoftware:master May 4, 2017

jfboismenu pushed a commit that referenced this pull request May 4, 2017

Revert "fix nested multi-entities query (#138)"

3187e01

This reverts commit 73554b5.

renaudll mentioned this pull request Nov 1, 2017

fix nested multi-entities query (take2) #155

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

renaudll commented Mar 7, 2017

Uh oh!

victoriagrey commented Mar 7, 2017

Uh oh!

jfboismenu commented Mar 13, 2017

Uh oh!

jfboismenu 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!

jfboismenu commented Mar 21, 2017

Uh oh!

renaudll commented Mar 22, 2017

Uh oh!

jfboismenu commented Mar 22, 2017

Uh oh!

renaudll commented Mar 31, 2017

Uh oh!

jfboismenu left a comment

Choose a reason for hiding this comment

Uh oh!

jfboismenu Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfboismenu Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jfboismenu Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

renaudll commented Apr 19, 2017

Uh oh!

jfboismenu 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!

renaudll Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jfboismenu Mar 31, 2017 •

edited

Loading

jfboismenu Mar 31, 2017 •

edited

Loading

jfboismenu Mar 31, 2017 •

edited

Loading

renaudll Apr 28, 2017 •

edited

Loading