fuhrysteve / marshmallow-jsonschema

JSON Schema Draft v7 (http://json-schema.org/) formatting with marshmallow
MIT License
209 stars 72 forks source link

UnsupportedValueError on marshmallow.fields.Number #85

Closed yeralin closed 5 years ago

yeralin commented 5 years ago

Recently I noticed that marshmallow-jsonschema does not work with marshmallow.fields.Number field and raises UnsupportedValueError.

Is there a particular reason why it does not? It works well with marshmallow.fields.Integer. Maybe we should just map it to Integer?

yeralin commented 5 years ago

Reproduced:

from marshmallow import Schema, fields
from marshmallow_jsonschema import JSONSchema

class TestSchema(Schema):
    test = fields.Number()

test_schema = TestSchema()

json_schema = JSONSchema()
json_schema.dump(test_schema).data

Traceback (most recent call last):
...
marshmallow_jsonschema.exceptions.UnsupportedValueError: unsupported field type <fields.Number(default=<marshmallow.missing>, attribute=None, validate=None, required=False, load_only=False, dump_only=False, missing=<marshmallow.missing>, allow_none=False, error_messages={'required': 'Missing data for required field.', 'type': 'Invalid input type.', 'null': 'Field may not be null.', 'validator_failed': 'Invalid value.', 'invalid': 'Not a valid number.', 'too_large': 'Number too large.'})>
fuhrysteve commented 5 years ago

Hmm, I'd think we'd probably want to map it to decimal.Decimal since that'll preserve fractional numbers & precision, right?

yeralin commented 5 years ago

I'll create a PR for that

yeralin commented 5 years ago

Actually, no it is coming from the original marshmallow code: https://github.com/marshmallow-code/marshmallow/blob/8cf1fb8d95f287d626ed0f38967c90198e28b476/src/marshmallow/schema.py#L285

It doesn't have a mapping for fields.Number

yeralin commented 5 years ago

However, then in marshmallow-jsonschema code, it "updates" the mapping by adding more mappings: https://github.com/fuhrysteve/marshmallow-jsonschema/blob/ee7fcaa543998b43452601767e3a6261ab84fe70/marshmallow_jsonschema/base.py#L93-L102

Not exactly sure why.

fuhrysteve commented 5 years ago

I don't remember why either. I think it may have been because someone wanted to be able to extend & override it more easily, you'd have to dig through the commit history a bit I guess to figure out why that mapping is so weirdly layed out and split apart.

yeralin commented 5 years ago

While I'm looking at the history, one of the straightforward solutions could be the following:

mapping.update( 
     { 
         ...
         fields.Number: decimal.Decimal,
     } 
 ) 

Then, the example above would produce:

{  
   '$schema':'http://json-schema.org/draft-07/schema#',
   '$ref':'#/definitions/TestSchema'
   'definitions':{  
      'TestSchema':{  
         'properties':{  
            'test':{  
               'title':'test',
               'type':'number',
               'format':'decimal'
            }
         },
         'type':'object',
         'additionalProperties':False
      }
   },
}
fuhrysteve commented 5 years ago

Yeah, that's what I had in mind as well.

yeralin commented 5 years ago

Hahahahaha, I tracked that change down all the way to the first initial commit. I guess it will be a mystery now: https://github.com/fuhrysteve/marshmallow-jsonschema/commit/81cebea8b66b9cc53bb7d04984de9f828154d375

What I think you were trying to do is to extend supported formats. You took the original TYPE_MAPPING from marshmallow package, and added more available fields to work with.

I think in that case my proposed solution is fine. I'll make a PR :)

Thanks Steve!

fuhrysteve commented 5 years ago

Who wrote that garbage?? :rofl:

I think it's because mapping is basically an inverse of TYPE_MAPPING and some of the values needed to be added computationally at run-time based on what's in the actual schema at hand.