Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added ability to parse json #73

Closed
wants to merge 1 commit into from
Closed

Conversation

elksson
Copy link

@elksson elksson commented Mar 2, 2021

No description provided.

@elksson
Copy link
Author

elksson commented Mar 2, 2021

This pull request should add support for parsing json so partially resolves one of the issues in #61

@elksson elksson changed the title Added ability to part json Added ability to parse json Mar 2, 2021
@pfalcon
Copy link
Owner

pfalcon commented Mar 2, 2021

Thanks for the submission.

so partially resolves one of the issues in #61

But did you read that ticket? Because if you did (and also read README), you'd know that Picoweb is minimalist web framework which doesn't duplicate functionality already available in other modules/libraries. You can parse json using ujson.loads(string).

@elksson
Copy link
Author

elksson commented Mar 4, 2021

Then by your own definition the function should not parse the form data since the user could just call parse_qs(data). The problem is that while parsing json is supported by another module the pico web module has functions to help with reading the response however the form data is read into a dictionary and parsed as a form instead of providing the raw string.

I would propose that if the framework is not going to provide a json read function then the function below should be modified so that the raw response is made available to be used

def read_form_data(self):
size = int(self.headers[b"Content-Length"])
data = yield from self.reader.readexactly(size)
form = parse_qs(data.decode())
self.form = form

Here are a few proposals

Option 1 - Save the (Raw Data) as a property on the Request so that it can be parsed by an external library without having to write a helper function for pico web module.

def read_form_data(self):
size = int(self.headers[b"Content-Length"])
self.data = yield from self.reader.readexactly(size)
form = parse_qs(self.data.decode())
self.form = form

Option 2 - Dont parse the data but return the raw response and let the calling application determine if its form data or json data.

def read_form_data(self):
size = int(self.headers[b"Content-Length"])
self.form = yield from self.reader.readexactly(size)

Using option 1 the web application can parse json by req.read_form_data()
then calling json.loads(req.data)

Using Option 2 The web application can parse the json by calling req.read_form_data()
then calling json.loads(req.form)

I would lean towards Option 1 because it maintains backwards compatibility without adding any lines of code but makes it very easy to parse the json while not having to re-write the code that reads the string from the request.

@elksson
Copy link
Author

elksson commented Mar 4, 2021

One other point is that if its such a bad idea to have a function in the HTTPRequest to parse the JSON Body which in todays day is a requirement on almost any web application then why is there a function called Jsonify provided by pico web which takes the dictionary and writes it to the response as Json?

Couldn’t the web application just call response.awrite(json.dumps(dict))?

@elksson
Copy link
Author

elksson commented Mar 4, 2021

Another 3rd option would be to define the function that reads the form data in such a way that it takes a parse data function as an agreement.

def read_form_data(self, parseFunction):
size = int(self.headers[b"Content-Length”])
data = yield from self.reader.readexactly(size)
self.form = parseFunction(data)

This allows the calling application to define how the data should be parsed (Json, Bson, Form Data, ect....) While still not having to deal with reading the raw response from the request.

@pfalcon
Copy link
Owner

pfalcon commented Mar 4, 2021

Then by your own definition

My own definition is very simple: Be conservative with both adding new things, and removing existing things. And as this is minimalist project, be extra conservative with adding new things.

the function should not parse the form data

Dealing with HTML form data is what almost any standard (classic) web app would do, and hence convenience function is provided.

why is there a function called Jsonify provided by pico web which takes the dictionary and writes it to the response as Json?

Because it's pretty common usecase to develop a webapp which exports REST API with JSON responses. So, a convenience function for that was provided.

I would propose that if the framework is not going to provide a json read function then the function below should be modified

Please understand that there's no need to modify anything. Instead you do:

class MyWebApp(WebApp):
    # Add here whatever methods you want.

If you find that almost all your webapps use such an extended subclass, you can put it in a separate module for easy reuse, and even share it with others and maintain it yourself (answer questions, etc.). I encourage all that. All that was mentioned in #61 (I keep it open exactly to let people know all that without repeating it again and again).

But picoweb itself is intended to stay minimalist, and allow to develop small/simple webapps without adding much extra code overhead, while also being extensible (like above) to allow developing more complex webapps.

Thanks for the discussion.

@pfalcon pfalcon closed this Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants