thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao Wang (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (THRIFT-4727) javascript compiler generates struct code with duplicate `case 0` statements
Date Mon, 07 Jan 2019 09:34:00 GMT

     [ https://issues.apache.org/jira/browse/THRIFT-4727?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Xiao Wang updated THRIFT-4727:
------------------------------
    Description: 
*Reproducible steps:*

Javascript compiler would always generate duplicate `case 0` codes;

1. Below is a demo IDL file:
{code:java}
// test.thrift
struct Obj {
   1: string name;
}

service Test {
   Obj test()
}

{code}
2. Generate code using
{code:java}
thrift -r --gen js:node test.thrift{code}
3. The related problematic code is like below:
{code:java}
// part of Test.js
......
Test_test_result.prototype.read = function(input) {
  input.readStructBegin();
  while (true)
  {
    var ret = input.readFieldBegin();
    var fname = ret.fname;
    var ftype = ret.ftype;
    var fid = ret.fid;
    if (ftype == Thrift.Type.STOP) {
      break;
    }
    switch (fid)
    {
      case 0:
      if (ftype == Thrift.Type.STRUCT) {
        this.success = new ttypes.Obj();
        this.success.read(input);
      } else {
        input.skip(ftype);
      }
      break;
      case 0:
        input.skip(ftype);
        break;
      default:
        input.skip(ftype);
    }
    input.readFieldEnd();
  }
  input.readStructEnd();
  return;
};
......{code}
*_Root cause:_*

The code makes this bug was imported due to bugfix THRIFT-1089.

Commit address: [https://github.com/apache/thrift/commit/da6e6ae91894bc52fc04234fbd2610f8969399f4]

*_Potential fix:_*

I think remove below code should fix this problem:
{code:java}
if (fields.size() == 1) {
  // pseudo case to make jslint happy
  indent(out) << "case 0:" << endl;
  indent(out) << " input.skip(ftype);" << endl;
  indent(out) << " break;" << endl;
}
{code}
This code seems to satisfy jslint about 8 years ago, but now this also fails the static code
scan both in jslint and sonarqube default profile.

I could propose a PR if it's OK to fix it this way.

 

  was:
*Reproducible steps:*

Javascript compiler would always generate duplicate `case 0` codes;

1. Below is a demo IDL file:

 
{code:java}
// test.thrift
struct Obj {
   1: string name;
}

service Test {
   Obj test()
}

{code}
2. Generate code using
{code:java}
thrift -r --gen js:node test.thrift{code}
3. The related problematic code is like below:

 

 
{code:java}
// part of Test.js
......
Test_test_result.prototype.read = function(input) {
  input.readStructBegin();
  while (true)
  {
    var ret = input.readFieldBegin();
    var fname = ret.fname;
    var ftype = ret.ftype;
    var fid = ret.fid;
    if (ftype == Thrift.Type.STOP) {
      break;
    }
    switch (fid)
    {
      case 0:
      if (ftype == Thrift.Type.STRUCT) {
        this.success = new ttypes.Obj();
        this.success.read(input);
      } else {
        input.skip(ftype);
      }
      break;
      case 0:
        input.skip(ftype);
        break;
      default:
        input.skip(ftype);
    }
    input.readFieldEnd();
  }
  input.readStructEnd();
  return;
};
......{code}
*_Root cause:_*

The code makes this bug was imported due to bugfix THRIFT-1089.

Commit address:[https://github.com/apache/thrift/commit/da6e6ae91894bc52fc04234fbd2610f8969399f4|https://github.com/apache/thrift/commit/da6e6ae91894bc52fc04234fbd2610f8969399f4.]

*_Potential fix:_*

I think remove below code should fix this problem:
{code:java}
if (fields.size() == 1) {
  // pseudo case to make jslint happy
  indent(out) << "case 0:" << endl;
  indent(out) << " input.skip(ftype);" << endl;
  indent(out) << " break;" << endl;
}
{code}
This code seems to satisfy jslint about 8 years ago, but now this also fails the static code
scan both in jslint and sonarqube default profile.

I could propose a PR if it's OK to fix it this way.

 


> javascript compiler generates struct code with duplicate  `case 0` statements
> -----------------------------------------------------------------------------
>
>                 Key: THRIFT-4727
>                 URL: https://issues.apache.org/jira/browse/THRIFT-4727
>             Project: Thrift
>          Issue Type: Bug
>          Components: JavaScript - Compiler
>    Affects Versions: 0.7, 0.8, 0.9, 0.10.0, 0.11.0
>            Reporter: Xiao Wang
>            Priority: Major
>
> *Reproducible steps:*
> Javascript compiler would always generate duplicate `case 0` codes;
> 1. Below is a demo IDL file:
> {code:java}
> // test.thrift
> struct Obj {
>    1: string name;
> }
> service Test {
>    Obj test()
> }
> {code}
> 2. Generate code using
> {code:java}
> thrift -r --gen js:node test.thrift{code}
> 3. The related problematic code is like below:
> {code:java}
> // part of Test.js
> ......
> Test_test_result.prototype.read = function(input) {
>   input.readStructBegin();
>   while (true)
>   {
>     var ret = input.readFieldBegin();
>     var fname = ret.fname;
>     var ftype = ret.ftype;
>     var fid = ret.fid;
>     if (ftype == Thrift.Type.STOP) {
>       break;
>     }
>     switch (fid)
>     {
>       case 0:
>       if (ftype == Thrift.Type.STRUCT) {
>         this.success = new ttypes.Obj();
>         this.success.read(input);
>       } else {
>         input.skip(ftype);
>       }
>       break;
>       case 0:
>         input.skip(ftype);
>         break;
>       default:
>         input.skip(ftype);
>     }
>     input.readFieldEnd();
>   }
>   input.readStructEnd();
>   return;
> };
> ......{code}
> *_Root cause:_*
> The code makes this bug was imported due to bugfix THRIFT-1089.
> Commit address: [https://github.com/apache/thrift/commit/da6e6ae91894bc52fc04234fbd2610f8969399f4]
> *_Potential fix:_*
> I think remove below code should fix this problem:
> {code:java}
> if (fields.size() == 1) {
>   // pseudo case to make jslint happy
>   indent(out) << "case 0:" << endl;
>   indent(out) << " input.skip(ftype);" << endl;
>   indent(out) << " break;" << endl;
> }
> {code}
> This code seems to satisfy jslint about 8 years ago, but now this also fails the static
code scan both in jslint and sonarqube default profile.
> I could propose a PR if it's OK to fix it this way.
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message