add: Wildcard fields #255

ziirish · 2017-03-15T16:05:27Z

This PR adds the ability to define models based on wildcard fields name as requested in #172.

coveralls · 2017-03-15T16:10:29Z

Coverage decreased (-0.3%) to 96.288% when pulling 8b6ec01 on ziirish:master into 9726fff on noirbizarre:master.

coveralls · 2017-03-20T10:59:18Z

Coverage decreased (-0.07%) to 96.502% when pulling bc2fb3e on ziirish:master into 9726fff on noirbizarre:master.

coveralls · 2017-03-20T12:28:30Z

Coverage decreased (-0.1%) to 96.845% when pulling d64f1c6 on ziirish:master into 00d79c1 on noirbizarre:master.

noirbizarre

To me, according to the swagger specs, we are exactly in the case of additionalProperties. They are not properly documented in the swagger specification, they appear only once in the schema section.
Here is a Stackoverflow post explaining it's behavior.
So we need something else to map to additionnalProperties.

Proposal: instead of a field definition, we allow an optionnal third parameter to model factory to define additionnalProperties.
An we need to improve marshalling performance. Here the impact is way to high for models not using it.

Available on gitter to discuss this.

noirbizarre · 2017-03-29T08:19:34Z

tests/test_fields.py

+    def test_defaults(self):
+        field = fields.Wildcard(fields.String)
+        assert not field.required
+        assert field.__schema__ == {'type': 'array', 'items': {'type': 'string'}}


Hum, I don't think the resulting data is an array. I think we need to make use of additionalProperties. See my review comment for details.

noirbizarre · 2017-03-29T08:20:55Z

flask_restplus/marshalling.py

+        keys.append(key)
+        items.append((key, value))
+
+    items = tuple(items)


I'm sorry but I can't let that happen this way. I'm currently trying to improve serialization performances which dropped in last releases and this is masively reducing it. We need to find another way, something more efficient.
(I'm talking about the whole marshalling changeset, not the last line)

I understand the issue here, but the Wildcard thing will require deep introspection and thus, people can/may/should expect slow results.

noirbizarre · 2017-03-29T08:32:23Z

flask_restplus/fields.py

+        else:
+            self._flat = []
+            attributes = inspect.getmembers(obj, lambda a: not(inspect.isroutine(a)))
+            for (key, val) in [a for a in attributes if not(a[0].startswith('__') and a[0].endswith('__'))]:


I think this could be done in a single iteration:
Something like:

def match_attribute(attr): # defined once outside return not(inspect.isroutine(attr) or (attr.__name__.startswith('__') and attr.__name__.endswith('__'))) self._flat.extend(inspect.getmembers(obj, match_attributes))

This is not completely doable, however, I managed to improve this a bit.

ziirish · 2018-05-25T15:05:59Z

Hi,

I tried to rework the Wildcard marshalling in order to reduce performance impact.
I had to make a trade-off though: Wildcards are going to be slower than before.

The impact should be null for any other fields.

Also, I would definitely need some help about the additionalProperties thing since I know nothing about Swagger specs.

ziirish · 2018-05-25T18:19:02Z

Here are the benchmark results on my development machine with the Wildcards patch:

------------------------------------------------------------------------------- benchmark 'marshalling': 4 tests -------------------------------------------------------------------------------
Name (time in ms)                     Min                Max              Mean            StdDev            Median               IQR            Outliers       OPS            Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bench_marshal_simple               1.7760 (1.0)       2.3401 (1.0)      1.8568 (1.0)      0.0593 (1.0)      1.8580 (1.0)      0.0720 (1.0)        255;11  538.5724 (1.0)         779           1
bench_marshal_simple_with_mask     2.5980 (1.46)      3.2241 (1.38)     2.7159 (1.46)     0.0787 (1.33)     2.7020 (1.45)     0.1040 (1.44)       236;10  368.2068 (0.68)        631           1
bench_marshal_nested               7.0601 (3.98)     20.6509 (8.82)     7.4630 (4.02)     1.0491 (17.70)    7.3400 (3.95)     0.1435 (1.99)         5;11  133.9948 (0.25)        252           1
bench_marshal_nested_with_mask     7.8800 (4.44)     14.5209 (6.21)     8.2509 (4.44)     0.6024 (10.17)    8.1691 (4.40)     0.1520 (2.11)          3;6  121.1984 (0.23)        232           1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

------------------------------------------------------------------------------------------ benchmark 'swagger': 2 tests ------------------------------------------------------------------------------------------
Name (time in us)                     Min                    Max                  Mean                StdDev                Median                 IQR            Outliers       OPS            Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bench_swagger_specs_cached       790.8344 (1.0)      17,014.0266 (1.73)     1,756.0156 (1.0)      2,845.7153 (17.20)      797.9870 (1.0)       27.6566 (1.0)         23;46  569.4710 (1.0)         224           1
bench_swagger_specs            8,264.0648 (10.45)     9,829.9980 (1.0)      8,460.3579 (4.82)       165.4908 (1.0)      8,411.8843 (10.54)    100.1358 (3.62)        41;25  118.1983 (0.21)        262           1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

And here are the benchmark results on my development machine without the Wildcards patch:

------------------------------------------------------------------------------- benchmark 'marshalling': 4 tests -------------------------------------------------------------------------------
Name (time in ms)                     Min                Max              Mean            StdDev            Median               IQR            Outliers       OPS            Rounds  Iterations
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bench_marshal_simple               1.7679 (1.0)       2.4860 (1.0)      1.8571 (1.0)      0.0677 (1.0)      1.8520 (1.0)      0.0813 (1.0)        206;15  538.4733 (1.0)         793           1
bench_marshal_simple_with_mask     2.5811 (1.46)     23.9642 (9.64)     3.0444 (1.64)     1.8513 (27.34)    2.7101 (1.46)     0.1099 (1.35)        24;43  328.4713 (0.61)        678           1
bench_marshal_nested               7.2360 (4.09)     15.1179 (6.08)     7.5575 (4.07)     0.5459 (8.06)     7.5084 (4.05)     0.1385 (1.70)          2;5  132.3195 (0.25)        256           1
bench_marshal_nested_with_mask     8.0450 (4.55)     16.3648 (6.58)     8.3908 (4.52)     0.6097 (9.00)     8.3110 (4.49)     0.1673 (2.06)         7;12  119.1783 (0.22)        233           1
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

----------------------------------------------------------------------------------------- benchmark 'swagger': 2 tests -----------------------------------------------------------------------------------------
Name (time in us)                     Min                   Max                  Mean              StdDev                Median                IQR            Outliers         OPS            Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bench_swagger_specs_cached       784.1587 (1.0)      1,046.8960 (1.0)        806.4524 (1.0)       35.2246 (1.0)        792.9802 (1.0)      18.3582 (1.0)         25;26  1,239.9988 (1.0)         229           1
bench_swagger_specs            8,249.0444 (10.52)    8,926.8684 (8.53)     8,460.5634 (10.49)    114.6325 (3.25)     8,440.9714 (10.64)    87.9765 (4.79)        64;31    118.1954 (0.10)        280           1
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

ziirish · 2018-05-28T09:58:18Z

Hi,

I finally tried to apply the requested change for the schema definition.

Here is the result:

        "Languages":{
            "properties":{
                "*":{
                    "type":"object",
                    "description":"Supported languages",
                    "additionalProperties":{
                        "type":"string"
                    }
                }
            },
            "type":"object"
        }

Using the following model:

    wild = fields.Wildcard(fields.String, description='Supported languages')                                                                           
    languages_fields = ns.model('Languages', {                                                                                                         
        '*': wild,                                                                                                                                     
    })

I think this is not 100% correct yet though. We should skip one level and have something like this instead:

        "Languages":{
            "type":"object",
            "description":"Supported languages",
            "additionalProperties":{
              "type":"string"
            }
        }

But this would mean the field type Wildcard is a kind of model itself and thus we should not allow any other fileds in the model.

Thoughts?

ziirish · 2018-05-28T14:17:22Z

I tried to improve wildcard parsing performances. I don't have any benchmark but I expect results to be slightly better.

andrei-apple · 2018-08-20T17:58:31Z

flask_restplus/fields.py

+        if obj == self._obj and self._flat is not None:
+            return self._flat
+        if isinstance(obj, dict):
+            self._flat = [x for x in obj.items()]


Ideally, should use six.viewitems() for Python 2x3 compatibility.

ziirish · 2018-09-21T09:13:26Z

Any news?

noirbizarre · 2018-09-21T16:42:53Z

To be honnest I can't currently merge this PR:

introduces a major signature change on marshal (which will break any customization on lib users, into which I am)
introduces a major performance hit

I like the feature but we need to find a way to properly integrate it without breaking the current behavior and without performances changes for users not using it.

I'm ready to discuss it. Don't hesite to PM me on gitter so we can discuss it

ziirish · 2018-09-21T16:57:18Z

What performance hit? I listened to your review and then corrected my implementation accordingly. The behavior remains unchanged for anything else then the wildcard field.

… 'container' instead

add: Wildcard fields

8b6ec01

improve test coverage + fix object introspection

bc2fb3e

improve test coverage + improve wildcard documentation

84566bd

noirbizarre requested changes Mar 29, 2017

View reviewed changes

rework wildcard marshalling to reduce performance impact

e6a0bc0

improve flatten wildcards

46529d1

ziirish added 2 commits May 25, 2018 22:26

improve test coverage + fix skip_none handling in wildcard marshalling

f6accf2

implement proper swagger schema for wildcards

6a81133

improve wildcard parsing performances by dropping its complexity to O(n)

e4e2ebc

ziirish mentioned this pull request Jul 25, 2018

How to document a map or dictionary using Models? #499

Open

andrei-apple reviewed Aug 20, 2018

View reviewed changes

ziirish added 4 commits September 27, 2018 15:43

python 2 compatibility

424649d

mask on the Wildcard field is unnecessary, you should use mask on the…

dc1783b

… 'container' instead

improve test coverage

d6981c6

fix linter issues

d64f1c6

noirbizarre approved these changes Oct 1, 2018

View reviewed changes

noirbizarre added this to the 0.13.0 milestone Oct 1, 2018

noirbizarre added the enhancement label Oct 1, 2018

noirbizarre merged commit a8f3582 into noirbizarre:master Oct 1, 2018

ziirish mentioned this pull request Feb 17, 2020

Wildcard field contains an extra nested level in docs python-restx/flask-restx#57

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add: Wildcard fields #255

add: Wildcard fields #255

ziirish commented Mar 15, 2017

coveralls commented Mar 15, 2017 •

edited

Loading

coveralls commented Mar 20, 2017 •

edited

Loading

coveralls commented Mar 20, 2017 •

edited

Loading

noirbizarre left a comment •

edited

Loading

noirbizarre Mar 29, 2017

noirbizarre Mar 29, 2017 •

edited

Loading

ziirish Mar 29, 2017

noirbizarre Mar 29, 2017

ziirish May 25, 2018

ziirish commented May 25, 2018

ziirish commented May 25, 2018

ziirish commented May 28, 2018

ziirish commented May 28, 2018

andrei-apple Aug 20, 2018

ziirish commented Sep 21, 2018

noirbizarre commented Sep 21, 2018

ziirish commented Sep 21, 2018

add: Wildcard fields #255

add: Wildcard fields #255

Conversation

ziirish commented Mar 15, 2017

coveralls commented Mar 15, 2017 • edited Loading

coveralls commented Mar 20, 2017 • edited Loading

coveralls commented Mar 20, 2017 • edited Loading

noirbizarre left a comment • edited Loading

Choose a reason for hiding this comment

noirbizarre Mar 29, 2017

Choose a reason for hiding this comment

noirbizarre Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

ziirish Mar 29, 2017

Choose a reason for hiding this comment

noirbizarre Mar 29, 2017

Choose a reason for hiding this comment

ziirish May 25, 2018

Choose a reason for hiding this comment

ziirish commented May 25, 2018

ziirish commented May 25, 2018

ziirish commented May 28, 2018

ziirish commented May 28, 2018

andrei-apple Aug 20, 2018

Choose a reason for hiding this comment

ziirish commented Sep 21, 2018

noirbizarre commented Sep 21, 2018

ziirish commented Sep 21, 2018

coveralls commented Mar 15, 2017 •

edited

Loading

coveralls commented Mar 20, 2017 •

edited

Loading

coveralls commented Mar 20, 2017 •

edited

Loading

noirbizarre left a comment •

edited

Loading

noirbizarre Mar 29, 2017 •

edited

Loading