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

postgres: add column name to the result of query function. #3201

Closed
wants to merge 8 commits into from

Conversation

Jack397419
Copy link

@Jack397419 Jack397419 commented Oct 30, 2023

add column name to the result of function “query”.

Description

Please explain the changes you've made

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

add column name to the result of function query.
@Jack397419 Jack397419 requested review from a team as code owners October 30, 2023 02:04
@Jack397419 Jack397419 changed the title Update postgres.go postgres: add column name to the result of query function. Oct 30, 2023
@@ -186,13 +186,21 @@ func (p *Postgres) query(ctx context.Context, sql string, args ...any) (result [
return nil, fmt.Errorf("error executing query: %w", err)
}

cols := rows.FieldDescriptions()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please run the Go formatting tool (gofumpt).


r := map[string]interface{}{}
for i, col := range cols {
r[string(col.Name)] = val[i]
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, this is a breaking change. Currently, users expect to receive a slice, but with this, they will receive a map. This will break all existing users.

Perhaps this should be a configurable option?

Copy link
Author

Choose a reason for hiding this comment

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

Infact, the query function return a json, not a map or a slice: "result, err = json.Marshal(rs)", and the json should include column name,like the query function in mysql.go: "result, err := m.jsonify(rows)"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, my point is: that is a breaking change. If users expect the JSON to contain an array, and you're now returning a dictionary, that will break applications.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with your opinion, and we expect the JSON contain a dictionary,how to make a configurable option? Can you help me?

Copy link
Member

Choose a reason for hiding this comment

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

You need to do this via request metadata which a user would need to specify to enable your new functionality, or introduce a new operation code. You can look at the code of other bindings to see how this can be done.

@berndverst
Copy link
Member

@Jack397419 can you fix the linter issue, and also fix DCO?

For DCO:
To add your Signed-off-by line to every commit in this branch:

Ensure you have a local copy of your branch by checking out the pull request locally via command line.
In your local branch, run: git rebase HEAD~7 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin master

@yaron2
Copy link
Member

yaron2 commented Dec 6, 2023

ping @Jack397419

@ItalyPaleAle ItalyPaleAle added the do-not-merge PR is not ready for merging label Dec 18, 2023
@ItalyPaleAle
Copy link
Contributor

This change is now part of #3282 which is implemented more correctly. I'm going to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge PR is not ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants