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

#1388 pgtype.Range json marshal and unmarshal #1408

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acim
Copy link

@acim acim commented Dec 1, 2022

No description provided.

@jackc
Copy link
Owner

jackc commented Dec 2, 2022

Is it possible to reuse some more of the existing text encoding and parsing code? I haven't looked at it in detail, but naively I would have assumed that MarshalJSON would basically delegate to the existing text format encoding code and UnmarshalJSON would basically delegate to the existing text format parsing code.

@acim
Copy link
Author

acim commented Dec 2, 2022

I could reuse parseUntypedTextRange in UnmarshalJSON, but reusing Encode method of encodePlanRangeCodecRangeValuerToText struct looks not an easy thing to do because it's using type OID's for which I don't see the way to get in func (src Range[T]) MarshalJSON() ([]byte, error) since T in Range is of type any. There are no further bounds, so getting an OID seems impossible.

		lowerPlan := plan.m.PlanEncode(plan.rc.ElementType.OID, TextFormatCode, lower)
		if lowerPlan == nil {
			return nil, fmt.Errorf("cannot encode %v as element of range", lower)
		}

		buf, err = lowerPlan.Encode(lower, buf)
		if err != nil {
			return nil, fmt.Errorf("failed to encode %v as element of range: %v", lower, err)
		}
		if buf == nil {
			return nil, fmt.Errorf("Lower cannot be NULL unless LowerType is Unbounded")
		}

and the similar piece of code for upper bound in case of json could be simple replaced by marshal. So the only useful code from there would be before and after these mentioned.

@acim
Copy link
Author

acim commented Dec 4, 2022

Actually, I found a way to reuse the encodePlanRangeCodecRangeValuerToText.Encode but I had to modify it to:

type encodePlanRangeCodecRangeValuerToText struct {
	rc Codec
	m  interface {
		PlanEncode(oid uint32, format int16, value any) EncodePlan
	}
}

I implemented tests for Range[Date] and Range[Numeric] and then found out that Numeric didn't have json.Unmarshaler implementation so I did that also. I couldn't run all tests because lot of them require database connection. @jackc, could you please allow CI to run for this PR?

@acim acim marked this pull request as ready for review December 4, 2022 17:13
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