Paths

Table of Contentst

Differential D7874

Add a schema type for handling binary strings
ClosedPublic
Actions

Authored by jayeshv on May 20 2022, 3:32 PM.

Details

Reviewers

anlambert

Group Reviewers

Reviewers

Commits

rDGQL0cabd8ee94b7: Add a schema type for handling binary strings

Summary

A type with two fields, text and base64, is returned for every binary string.
Any decode error in the text field will be ignored.

Diff Detail

Repository

rDGQL GraphQL API

Branch

binary-string

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 29515
Build 46120: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 46119: arc lint + arc unit

Event Timeline

jayeshv created this revision.May 20 2022, 3:32 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptMay 20 2022, 3:32 PM

Build is green

Patch application report for D7874 (id=28423)

Rebasing onto d5010fd75f...

Current branch diff-target is up to date.

Changes applied before test

commit e03f4cac16f004ee03920aae032c429ef0b208e1
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon May 16 12:22:49 2022 +0200

    Add a schema type for handling binary strings
    
    A type with two fields, text and base64, is returned for every binary string.
    Any decode error in the text field will be ignored.

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/25/ for more details.

Harbormaster completed remote builds in B29514: Diff 28423.May 20 2022, 3:34 PM

jayeshv requested review of this revision.May 20 2022, 3:34 PM

Remove unused code

Build is green

Patch application report for D7874 (id=28424)

Rebasing onto d5010fd75f...

Current branch diff-target is up to date.

Changes applied before test

commit cb1f34271867484b8c25fe5d5dfdcb624d6ea369
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon May 16 12:22:49 2022 +0200

    Add a schema type for handling binary strings
    
    A type with two fields, text and base64, is returned for every binary string.
    Any decode error in the text field will be ignored.

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/26/ for more details.

Harbormaster completed remote builds in B29515: Diff 28424.May 20 2022, 3:39 PM

vlorentz added a subscriber: vlorentz.May 20 2022, 4:17 PM

vlorentz added inline comments.

swh/graphql/schema/schema.graphql
52
113	btw, they are actually IRIs, not URLs; but we can fix that later
379	how does it work to filter on non-unicode strings?

jayeshv added inline comments.May 20 2022, 4:33 PM

swh/graphql/schema/schema.graphql
379	@vlorentz We can accept the same object here. User can either use 'text' or a 'base64' string. Is that really needed, how often we get such filters, should we add complexity for a rather rare case?

anlambert requested changes to this revision.May 20 2022, 4:35 PM

anlambert added a subscriber: anlambert.

anlambert added inline comments.

swh/graphql/resolvers/resolvers.py
252	Use swh.core.utils.decode_with_escape instead. This is what is currently used in REST API to attempt decoding UTF-8 strings from bytes. It has the advantage to escape bytes of invalid utf-8 sequences instead of discarding them so users can see something went wrong while attempting decoding such strings.
swh/graphql/utils/utils.py
12	I do not think you need a variable for this, just put the "utf-8" literal directly in the code.
18	`.decode("ascii")`

This revision now requires changes to proceed.May 20 2022, 4:35 PM

vlorentz added inline comments.May 20 2022, 4:36 PM

swh/graphql/schema/schema.graphql
379	I don't know; probably not. On the other hand, it forces users to think about that case.

vlorentz added inline comments.May 20 2022, 4:37 PM

swh/graphql/resolvers/resolvers.py
252	so users can see something went wrong we might as well use `.decode(utils.ENCODING, "replace")` then; that's what it is for

anlambert added inline comments.May 20 2022, 4:41 PM

swh/graphql/resolvers/resolvers.py
252	correct me if I am wrong but that error handler replaces invalid Unicode character by the U+FFFD character, it will not backslash escape the bytes
252	oh OK got it, it is more visual with that character that something went wrong

Addressing review comments

jayeshv marked 3 inline comments as done.May 20 2022, 5:09 PM

jayeshv added inline comments.

swh/graphql/utils/utils.py
12	This is used in resolvers.py as well.

Build is green

Patch application report for D7874 (id=28435)

Rebasing onto d5010fd75f...

Current branch diff-target is up to date.

Changes applied before test

commit 0cabd8ee94b7100b8ea3d3949f81f8529d34d971
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon May 16 12:22:49 2022 +0200

    Add a schema type for handling binary strings
    
    A type with two fields, text and base64, is returned for every binary string.
    Any decode error in the text field will be ignored.

See https://jenkins.softwareheritage.org/job/DGQL/job/tests-on-diff/27/ for more details.

Harbormaster completed remote builds in B29526: Diff 28435.May 20 2022, 5:09 PM

This revision was not accepted when it landed; it landed in state Needs Review.May 24 2022, 1:55 PM

Closed by commit rDGQL0cabd8ee94b7: Add a schema type for handling binary strings (authored by jayeshv). · Explain Why

This revision was automatically updated to reflect the committed changes.

jayeshv added a commit: rDGQL0cabd8ee94b7: Add a schema type for handling binary strings.