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

fix: json decode #927

Closed
wants to merge 0 commits into from
Closed

Conversation

asamaayako
Copy link
Contributor

@asamaayako asamaayako commented Aug 13, 2024

Description

This PR fixes #816

Summary

This PR add a function json_string_to_pyobject in requst.rs . It's converts json_string to Python object.
I'm not sure if it's appropriate to put this function in this location.

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

Copy link

vercel bot commented Aug 13, 2024

@asamaayako is attempting to deploy a commit to the sparckles Team on Vercel.

A member of the Team first needs to authorize it.

@asamaayako asamaayako changed the title Fix/json decode fix: json decode Aug 13, 2024
Copy link

codspeed-hq bot commented Aug 13, 2024

CodSpeed Performance Report

Merging #927 will not alter performance

Comparing asamaayako:fix/json_decode (2703e06) with main (1d555c9)

Summary

✅ 110 untouched benchmarks

src/types/request.rs Outdated Show resolved Hide resolved
src/types/request.rs Outdated Show resolved Hide resolved
@asamaayako
Copy link
Contributor Author

Hey! @sansyrox I'm ready to be merged, if you think it's okay。

@sansyrox
Copy link
Member

Hey @asamaayako 👋

Reviewing!

jsondict = request.json()
except ValueError:
jsondict = None
return jsonify(jsondict)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just return a dict here. No need to use jsonify

Copy link
Contributor Author

@asamaayako asamaayako Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just return a dict here. No need to use jsonify

I try just return dict , but it error, jsondict is not always a dict. it Maybe a list.
when return this value:

jsondict = [{'k': 'v'}, {'k': None}]

it response.body is :

[{'k': 'v'}, {'k': None}]

This is not a valid JSON string.
the valid JSON is that:

[{"k":"v"},{"k":null}]

so , I used jsonify,


dict.set_item(py_key, py_value)?;
}
///serde_json not impl IntoPy<PyObject> but orphan principle. so use this function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asamaayako , what is the orphan principle ? Could you please explain?

Copy link
Contributor Author

@asamaayako asamaayako Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asamaayako , what is the orphan principle ? Could you please explain?

sorry, I means Orphan Rule. I can't impl IntoPy for serde_json in this create. because either the trait IntoPy or the type serde_json must be defined in the current crate.

Comment on lines 268 to 281
fn number_to_pyobject(num: serde_json::Number, py: Python) -> PyResult<PyObject> {
match num.as_u64() {
Some(u) => Ok(u.into_py(py)),
None => match num.as_i64() {
Some(i) => Ok(i.into_py(py)),
None => match num.as_f64() {
Some(f) => Ok(f.into_py(py)),
None => Err(PyErr::new::<pyo3::exceptions::PyTypeError, _>(
"Invalid number format",
)),
},
},
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this handle -1 ?

Copy link
Contributor Author

@asamaayako asamaayako Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 will be python int(-1).
It wil handle by num.as_i64() case N::NegInt(n) => Some(n), it will into to Python int .
In this function . try parse to uint failed , try parse to int failed , try parse to float ,

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.

Incorrect Data Types in JSON Serialization from Python Client to Server
2 participants