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: paramaters of type character and bit not ignoring length #2867

Merged
merged 6 commits into from
Jul 31, 2023

Conversation

laurenceisla
Copy link
Member

Closes #1586.

@laurenceisla
Copy link
Member Author

According to PostgreSQL docs on character types:

If specified, the length must be greater than zero and cannot exceed 10485760. character without length specifier is equivalent to character(1)

But, the docs for CREATE FUNCTION say this:

The full SQL type syntax is allowed for declaring a function's arguments and return value. However, parenthesized type modifiers (e.g., the precision field for type numeric) are discarded by CREATE FUNCTION. Thus for example CREATE FUNCTION foo (varchar(10)) ... is exactly the same as CREATE FUNCTION foo (varchar) ...

It made me think that CREATE FUNCTION foo(character(10)) is the same as CREATE FUNCTION foo(character), where character is of length 1 by default, as the docs on chars say. But that's not the case, because it allows to use parameters with a maximum length specified for a character which is the same of a varchar and text (10485760).

I couldn't find in the source code how they handle this change, so I casted both character and bit to their character varying and bit varying respectively. I'm not sure if it's a bit opinionated on my part.

@steve-chavez
Copy link
Member

steve-chavez commented Jul 22, 2023

so I casted both character and bit to their character varying and bit varying respectively

If they fail when exceeding the length I guess it's fine. (maybe the error message changes but it wouldnt be a dealbreaker)

Comment on lines +73 to +74
, ppType :: Text
, ppTypeMaxLength :: Text
Copy link
Member

Choose a reason for hiding this comment

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

Is ppType now unused? Would it make sense to have a single ppType with the added length for these cases?

Copy link
Member Author

@laurenceisla laurenceisla Jul 25, 2023

Choose a reason for hiding this comment

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

No, ppType is still used on every other place (e.g. errors, OpenAPI, etc.) Only when building the Query it transforms e.g. character -> character varying.

The thing is, the length is ignored by PostgreSQL, it's not specified in the function definition, so no length can be added, it's the character -> character varying change that's done here.

To clarify a little more, this change could also be done using only ppType. It wouldn't be at Schema Cache level, but per request instead, e.g.

case ppType of
  "character" -> "character varying"
  "bit" -> "bit varying"
  ...

@laurenceisla
Copy link
Member Author

Interestingly, only the PG14 io-test keeps failing. Could not find a reason why for now. Maybe a change needs to be done in the schema query.

@steve-chavez
Copy link
Member

@laurenceisla I don't see the pg 14 failure from CI. Can you link to the error?

@laurenceisla
Copy link
Member Author

@steve-chavez Ah, true. Strangely it failed twice in a row (1st and 2nd attempt) https://github.com/PostgREST/postgrest/actions/runs/5661060491/attempts/2?pr=2867, then the last ones run without problems.

@laurenceisla laurenceisla merged commit 40c2bcd into PostgREST:main Jul 31, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

postgrest: value too long for type character(1)
2 participants