Skip to content

Commit

Permalink
feat: add strict option to CLI (#119)
Browse files Browse the repository at this point in the history
Instead of throwing when encountering proto2 syntax, log a warning.

Add a `--strict` option to turn this warning into an error.

This is required to support [custom options](https://protobuf.dev/programming-guides/proto3/#customoptions)
which can necessitate compiling built in proto2 definitions (e.g. `extend google.protobuf.FileOptions`).
  • Loading branch information
achingbrain authored Oct 23, 2023
1 parent e86d817 commit 8c039c5
Show file tree
Hide file tree
Showing 18 changed files with 655 additions and 246 deletions.
5 changes: 5 additions & 0 deletions packages/protons/bin/protons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ async function main (): Promise<void> {
Options
--output, -o Path to a directory to write transpiled typescript files into
--strict, -s Causes parsing warnings to become errors
Examples
$ protons ./path/to/file.proto ./path/to/other/file.proto
Expand All @@ -20,6 +21,10 @@ async function main (): Promise<void> {
output: {
type: 'string',
shortFlag: 'o'
},
strict: {
type: 'boolean',
shortFlag: 's'
}
}
})
Expand Down
94 changes: 78 additions & 16 deletions packages/protons/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ function pathWithExtension (input: string, extension: string, outputDir?: string
return path.join(output, path.basename(input).split('.').slice(0, -1).join('.') + extension)
}

export class CodeError extends Error {
public code: string

constructor (message: string, code: string, options?: ErrorOptions) {
super(message, options)

this.code = code
}
}

const types: Record<string, string> = {
bool: 'boolean',
bytes: 'Uint8Array',
Expand Down Expand Up @@ -287,6 +297,13 @@ interface FieldDef {
map: boolean
valueType: string
keyType: string

/**
* Support proto2 required field. This field means a value should always be
* in the serialized buffer, any message without it should be considered
* invalid. It was removed for proto3.
*/
proto2Required: boolean
}

function defineFields (fields: Record<string, FieldDef>, messageDef: MessageDef, moduleDef: ModuleDef): string[] {
Expand All @@ -299,10 +316,22 @@ function defineFields (fields: Record<string, FieldDef>, messageDef: MessageDef,
})
}

function compileMessage (messageDef: MessageDef, moduleDef: ModuleDef): string {
function compileMessage (messageDef: MessageDef, moduleDef: ModuleDef, flags?: Flags): string {
if (isEnumDef(messageDef)) {
moduleDef.imports.add('enumeration')

// check that the enum def values start from 0
if (Object.values(messageDef.values)[0] !== 0) {
const message = `enum ${messageDef.name} does not contain a value that maps to zero as it's first element, this is required in proto3 - see https://protobuf.dev/programming-guides/proto3/#enum`

if (flags?.strict === true) {
throw new CodeError(message, 'ERR_PARSE_ERROR')
} else {
// eslint-disable-next-line no-console
console.info(`[WARN] ${message}`)
}
}

return `
export enum ${messageDef.name} {
${
Expand Down Expand Up @@ -332,7 +361,7 @@ export namespace ${messageDef.name} {
if (messageDef.nested != null) {
nested = '\n'
nested += Object.values(messageDef.nested)
.map(def => compileMessage(def, moduleDef).trim())
.map(def => compileMessage(def, moduleDef, flags).trim())
.join('\n\n')
.split('\n')
.map(line => line.trim() === '' ? '' : ` ${line}`)
Expand Down Expand Up @@ -391,13 +420,25 @@ export interface ${messageDef.name} {

if (fieldDef.map) {
valueTest = `obj.${name} != null && obj.${name}.size !== 0`
} else if (!fieldDef.optional && !fieldDef.repeated) {
} else if (!fieldDef.optional && !fieldDef.repeated && !fieldDef.proto2Required) {
// proto3 singular fields should only be written out if they are not the default value
if (defaultValueTestGenerators[type] != null) {
valueTest = `${defaultValueTestGenerators[type](`obj.${name}`)}`
} else if (type === 'enum') {
// handle enums
valueTest = `obj.${name} != null && __${fieldDef.type}Values[obj.${name}] !== 0`
const def = findDef(fieldDef.type, messageDef, moduleDef)

if (!isEnumDef(def)) {
throw new Error(`${fieldDef.type} was not enum def`)
}

valueTest = `obj.${name} != null`

// singular enums default to 0, but enums can be defined without a 0
// value which is against the proto3 spec but is tolerated
if (Object.values(def.values)[0] === 0) {
valueTest += ` && __${fieldDef.type}Values[obj.${name}] !== 0`
}
}
}

Expand Down Expand Up @@ -496,14 +537,16 @@ export interface ${messageDef.name} {
break
}`
} else if (fieldDef.repeated) {
return `case ${fieldDef.id}:
return `case ${fieldDef.id}: {
obj.${fieldName}.push(${parseValue})
break`
break
}`
}

return `case ${fieldDef.id}:
return `case ${fieldDef.id}: {
obj.${fieldName} = ${parseValue}
break`
break
}`
}

return createReadField(fieldName, fieldDef)
Expand Down Expand Up @@ -532,9 +575,10 @@ ${encodeFields === '' ? '' : `${encodeFields}\n`}
const tag = reader.uint32()
switch (tag >>> 3) {${decodeFields === '' ? '' : `\n ${decodeFields}`}
default:
default: {
reader.skipType(tag & 7)
break
}
}
}
Expand Down Expand Up @@ -570,7 +614,7 @@ interface ModuleDef {
globals: Record<string, ClassDef>
}

function defineModule (def: ClassDef): ModuleDef {
function defineModule (def: ClassDef, flags: Flags): ModuleDef {
const moduleDef: ModuleDef = {
imports: new Set(),
importedTypes: new Set(),
Expand All @@ -582,10 +626,10 @@ function defineModule (def: ClassDef): ModuleDef {
const defs = def.nested

if (defs == null) {
throw new Error('No top-level messages found in protobuf')
throw new CodeError('No top-level messages found in protobuf', 'ERR_NO_MESSAGES_FOUND')
}

function defineMessage (defs: Record<string, ClassDef>, parent?: ClassDef): void {
function defineMessage (defs: Record<string, ClassDef>, parent?: ClassDef, flags?: Flags): void {
for (const className of Object.keys(defs)) {
const classDef = defs[className]

Expand All @@ -603,9 +647,19 @@ function defineModule (def: ClassDef): ModuleDef {
fieldDef.repeated = fieldDef.rule === 'repeated'
fieldDef.optional = !fieldDef.repeated && fieldDef.options?.proto3_optional === true
fieldDef.map = fieldDef.keyType != null
fieldDef.proto2Required = false

if (fieldDef.rule === 'required') {
throw new Error('"required" fields are not allowed in proto3 - please convert your proto2 definitions to proto3')
const message = `field "${name}" is required, this is not allowed in proto3. Please convert your proto2 definitions to proto3 - see https://github.com/ipfs/protons/wiki/Required-fields-and-protobuf-3`

if (flags?.strict === true) {
throw new CodeError(message, 'ERR_PARSE_ERROR')
} else {
fieldDef.proto2Required = true

// eslint-disable-next-line no-console
console.info(`[WARN] ${message}`)
}
}
}
}
Expand Down Expand Up @@ -644,22 +698,30 @@ function defineModule (def: ClassDef): ModuleDef {
}
}

defineMessage(defs)
defineMessage(defs, undefined, flags)

// set enum/message fields now all messages have been defined
updateTypes(defs)

for (const className of Object.keys(defs)) {
const classDef = defs[className]

moduleDef.compiled.push(compileMessage(classDef, moduleDef))
moduleDef.compiled.push(compileMessage(classDef, moduleDef, flags))
}

return moduleDef
}

interface Flags {
/**
* Specifies an output directory
*/
output?: string

/**
* If true, warnings will be thrown as errors
*/
strict?: boolean
}

export async function generate (source: string, flags: Flags): Promise<void> {
Expand Down Expand Up @@ -701,7 +763,7 @@ export async function generate (source: string, flags: Flags): Promise<void> {
}
}

const moduleDef = defineModule(def)
const moduleDef = defineModule(def, flags)

const ignores = [
'/* eslint-disable import/export */',
Expand Down
6 changes: 6 additions & 0 deletions packages/protons/test/bad-fixtures/enum.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
syntax = "proto3";

enum AnEnum {
// enum values should start from 0
value1 = 1;
}
12 changes: 8 additions & 4 deletions packages/protons/test/fixtures/basic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,15 +47,18 @@ export namespace Basic {
const tag = reader.uint32()

switch (tag >>> 3) {
case 1:
case 1: {
obj.foo = reader.string()
break
case 2:
}
case 2: {
obj.num = reader.int32()
break
default:
}
default: {
reader.skipType(tag & 7)
break
}
}
}

Expand Down Expand Up @@ -99,9 +102,10 @@ export namespace Empty {
const tag = reader.uint32()

switch (tag >>> 3) {
default:
default: {
reader.skipType(tag & 7)
break
}
}
}

Expand Down
Loading

0 comments on commit 8c039c5

Please sign in to comment.