feat: add support to more postgres types issue#102#105
feat: add support to more postgres types issue#102#105debba merged 19 commits intoTabularisDB:mainfrom
Conversation
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
SUGGESTION
Notes on Previous ReviewThe previous review comments regarding
Files Reviewed (6 files)
Reviewed by kimi-k2.5-0127 · 555,131 tokens |
|
Yes looks ok to me , you can go ahead |
|
@dev-void-7 I will wait you complete tasks related for doing a review, let me know when is ready thanks |
|
Hi, My regards |
…odify `macro` to align with `tokio-postgres::Row` too
| let dimentions = dimentions as usize; | ||
|
|
||
| // each dimension must have at least 8 bytes info | ||
| if buf.len() * dimentions < 8 * dimentions { |
There was a problem hiding this comment.
CRITICAL: Incorrect buffer length check logic
The check buf.len() * dimentions < 8 * dimentions is algebraically equivalent to buf.len() < 8 (assuming dimensions > 0), which is incorrect.
At this point, buf still contains the 12-byte header. The check should ensure there are 8 * dimensions bytes remaining AFTER the header for dimension descriptors.
| if buf.len() * dimentions < 8 * dimentions { | |
| if buf.len() < 12 + 8 * dimentions { |
| } | ||
|
|
||
| pub fn split_at_value_len<'a>(buf: &mut &'a [u8]) -> Result<&'a [u8], ()> { | ||
| let len = i32::from_be_bytes(match buf[..4].try_into() { |
There was a problem hiding this comment.
CRITICAL: Missing bounds check before slicing
This will panic if buf has fewer than 4 bytes. Should check buf.len() < 4 first.
| let len = i32::from_be_bytes(match buf[..4].try_into() { | |
| if buf.len() < 4 { | |
| log::error!("Buffer too short: {}", buf.len()); | |
| return Err(()); | |
| } | |
| let len = i32::from_be_bytes(match buf[..4].try_into() { | |
| Ok(bytes) => bytes, | |
| Err(e) => { | |
| log::error!("Failed to parse value length: {:?}", e); | |
| return Err(()); | |
| } | |
| }); |
There was a problem hiding this comment.
no need to check because try_into will not panic, but will return Result instead,, in addition to the fact that Result is checked.
| return Err(()); | ||
| }; | ||
|
|
||
| let dimentions = i32::from_be_bytes(buf[..4].try_into().unwrap()); |
There was a problem hiding this comment.
SUGGESTION: Typo in variable name
dimentions should be dimensions. This typo appears throughout the file (lines 17, 20, 26, 31, 34, 61, 87).
| let dimentions = i32::from_be_bytes(buf[..4].try_into().unwrap()); | |
| let dimensions = i32::from_be_bytes(buf[..4].try_into().unwrap()); |
| #[inline] | ||
| pub fn extract_or_null(fields: &Vec<Field>, buf: &mut &[u8]) -> Result<JsonValue, ()> { | ||
| if buf.len() == 0 { | ||
| log::error!("recieved empty buffer"); |
There was a problem hiding this comment.
SUGGESTION: Typo in error message
recieved should be received.
| log::error!("recieved empty buffer"); | |
| log::error!("received empty buffer"); |
|
@debba hi,
inform me if there is any problem in the code. My Regards |
|
Hi @dev-void-7 , I tried to do some tests with array columns but always have: Error: db error
|
|
@debba |
|
@debba there is a syntax error with the query anyway this is related to the error message being unclear at all. i tried to fix this like that: what do you think? |
|
Yes looks great, it could be wonderful to have the message and a button for viewing full details what do you think? |
|
i think to implement this we should return a struct instead of anyway if we want to do that i think it should be in a separate PR. do you agree? i am going to push the commits i made so you can test things 😊. Thank you in advance |
|
Thanks, I will merge it :) |
|
Thank You very much this was my first PR 😊🎉🔥. I hope my contribution was helpful There is still some types that is not supported like My Regards |
|
The thanks go entirely to you; I’ll create a release between today and tomorrow. |
|
For sure i will continue collaborating specifically on have a nice day 🔥. |
|
I added my thanks here: https://tabularis.dev/blog/v0912-tokio-postgres-minimax-json-editor |


Supported Postgres Types
Limitation With
sqlxI think it is not possible to implement Nested composite types, Array of composite type or Nested arrays because
sqlxdoes not expose (does not make the public outside of the crate) important functions and methods to do thatSuggested Solution
Replace
sqlxwithtokio-postgresor the sync versionpostgresbecause they expose most of the functions needed to implement the features mentioned above.what do you think?