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.

Sign up for GitHub

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

Jump to bottom

model: correct the usage of json.Unmarshal in DecodeArgs #890

Merged
merged 5 commits into from
Jun 9, 2020
Merged

model: correct the usage of json.Unmarshal in DecodeArgs #890

merged 5 commits into from
Jun 9, 2020

Conversation

bobotu
Copy link
Contributor

@bobotu bobotu commented Jun 8, 2020

What problem does this PR solve?

Fixed #889

What is changed and how it works?

decoding into []json.RawMessage, so you can then decode each into the appropriate type in a second pass

Using json.Decoder is more complex.

if len(job.RawArgs) == 0 {
	return nil
}

decoder := json.NewDecoder(bytes.NewReader(job.RawArgs))
token, err := decoder.Token()
if err != nil {
	return errors.Trace(err)
}
if del, ok := token.(json.Delim); !ok || del != '[' {
	return nil
}

var i int
for decoder.More() {
	if i == len(args) {
		break
	}
	if err := decoder.Decode(args[i]); err != nil {
		return errors.Trace(err)
	}
	i++
}
job.Args = args[:i]
return nil

Check List

Tests

  • Manual test (add detailed scripts or steps below)
  • Run TiDB's unit-test with Go 1.15

Side effects

  • Possible performance regression

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@bobotu bobotu requested a review from tiancaiamao June 8, 2020 11:52
@bobotu bobotu added the type/bug-fix fix bug label Jun 8, 2020
@codecov
Copy link

codecov bot commented Jun 8, 2020 •
edited
Loading

Codecov Report

Merging #890 into master will decrease coverage by 0.01%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master     #890      +/-   ##
==========================================
- Coverage   78.32%   78.30%   -0.02%     
==========================================
  Files          40       40              
  Lines       14708    14716       +8     
==========================================
+ Hits        11520    11524       +4     
- Misses       2505     2507       +2     
- Partials      683      685       +2

@bobotu bobotu mentioned this pull request Jun 8, 2020
Merged
kennytm
kennytm reviewed Jun 8, 2020
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

Can you add a test case to ascertain this behavior?

Rest LGTM

model/ddl.go Outdated Show resolved Hide resolved
@bobotu
Copy link
Contributor Author

bobotu commented Jun 9, 2020 •
edited
Loading

@kennytm The existing test case already covered DecodeArgs, I have run UT with Go 1.15 to ensure it is correct.

@bobotu bobotu requested a review from kennytm June 9, 2020 03:23
wjhuang2016
wjhuang2016 reviewed Jun 9, 2020
View reviewed changes
model/ddl.go Outdated Show resolved Hide resolved
kennytm
kennytm reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

LGTM

wjhuang2016
wjhuang2016 reviewed Jun 9, 2020
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@kennytm kennytm added status/LGT2 LGT2 and removed status/LGT1 LGT1 labels Jun 9, 2020
jackysp
jackysp approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp
Copy link
Member

jackysp commented Jun 9, 2020

/run-build-integration

@jackysp jackysp merged commit c65941b into pingcap:master Jun 9, 2020
@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-2.1 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-3.1 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-4.0 failed

@bobotu
Copy link
Contributor Author

bobotu commented Jun 9, 2020

/run-cherry-picker

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-2.1 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-3.0 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-3.1 failed

@sre-bot
Copy link

sre-bot commented Jun 9, 2020

cherry pick to release-4.0 failed

This was referenced Jun 10, 2020
Merged
Merged
Merged
Merged
bobotu pushed a commit to bobotu/parser that referenced this pull request Jun 11, 2020
@bobotu
model: correct the usage of json.Unmarshal in DecodeArgs (pingcap#890)
6abdaab
@bobotu bobotu mentioned this pull request Jun 11, 2020
Merged
bobotu pushed a commit to bobotu/parser that referenced this pull request Jun 11, 2020
@bobotu
model: correct the usage of json.Unmarshal in DecodeArgs (pingcap#890)
3d8553a
@bobotu bobotu mentioned this pull request Jun 11, 2020
Merged
bobotu pushed a commit to bobotu/parser that referenced this pull request Jun 11, 2020
@bobotu
model: correct the usage of json.Unmarshal in DecodeArgs (pingcap#890)
bf4013e
@bobotu bobotu mentioned this pull request Jun 11, 2020
Merged
bobotu pushed a commit to bobotu/parser that referenced this pull request Jun 11, 2020
@bobotu
model: correct the usage of json.Unmarshal in DecodeArgs (pingcap#890)
16b38d4
@bobotu bobotu mentioned this pull request Jun 11, 2020
Merged
jackysp pushed a commit that referenced this pull request Jun 16, 2020
model: correct the usage of json.Unmarshal in DecodeArgs (#890) (#896)
4e23afb
bobotu pushed a commit that referenced this pull request Jun 16, 2020
model: correct the usage of json.Unmarshal in DecodeArgs (#890) (#897)
d170b9f
jackysp pushed a commit that referenced this pull request Jun 16, 2020
model: correct the usage of json.Unmarshal in DecodeArgs (#890) (#898)
4b5081d
bobotu pushed a commit that referenced this pull request Jun 16, 2020
@jackysp
model: correct the usage of json.Unmarshal in DecodeArgs (#890) (#895)
a87fd8a 
Co-authored-by: Jack Yu <[email protected]>
tiancaiamao pushed a commit to tiancaiamao/parser that referenced this pull request Apr 27, 2021
model: correct the usage of json.Unmarshal in DecodeArgs (pingcap#890)
647c8ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kennytm kennytm kennytm left review comments

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@jackysp jackysp jackysp approved these changes

@tiancaiamao tiancaiamao Awaiting requested review from tiancaiamao

Assignees
No one assigned
Labels
needs-cherry-pick-release-2.1 needs-cherry-pick-release-3.0 needs-cherry-pick-release-3.1 needs-cherry-pick-release-4.0 status/LGT2 LGT2 type/bug-fix fix bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DecodeArgs relies on a wrong behavior of json.Unmarshal
7 participants
@bobotu @CLAassistant @jackysp @sre-bot @kennytm @wjhuang2016 @tiancaiamao