Resolve ambiguity between descriptorFile and fileName when parsing descriptors

In 1.9.0 a new method getDescrptorFile was introduced. This actually can cause various problems and has some open design questions that should be answered in this ticket. (I'll attach a branch with a test demonstrating part of the problem.)

DescriptorParser has the new method (since 1.9.0):

public Iterable<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
    File descriptorFile, String fileName);

In addition to the problem demonstrated in the (to be attached) test this raises some questions:

1. What should be done, if descriptorFile.getName() != fileName?
2. What should be done, if descriptorFile != null && (null == fileName || fileName.isEmpty())?
3. What should be done, if descriptorFile == null && null != fileName? 4a) What should be done, if descriptorFile == null && null != fileName && fileName.isEmpty()? 4b) What should be done, if descriptorFile == null && null == fileName?

The implementation of the solution should also have tests for each of the possible cases.

changed milestone to %metrics-lib 2.3.0

added component::metrics/library metrics-2018 milestone::metrics-lib 2.3.0 owner::iwakeh priority::medium resolution::fixed severity::normal status::closed type::defect labels

The failing assertion is assertNotNull(desc.getDescriptorFile()); in this branch.

First thoughts about possible answers:

1. Several answers: i) some form of exception? ii) warning and use the descriptorFile given?
2. Set fileName = descriptorFile.getName().
3. More tricky: i) use descriptorFile = new File(fileName) is potentially problematic b/c there is no such file. ii) leave descriptorFile = null and forget about the given filename? iii) add getDescriptorFilename() to the Descriptor interface? iv) some form of exception?
4. some form of exception?

Trac:
Description: In 1.9.0 a new method getDescrptorFile was introduced. This actually can cause various problems and has some open design questions that should be answered in this ticket. (I'll attach a branch with a test demonstration part of the problem.)

DescriptorParser has the new method (since 1.9.0):

public Iterable<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
    File descriptorFile, String fileName);

In addition to the problem demonstrated in the (to be attached) test this raises some questions:

1. What should be done, if descriptorFile.getName() != fileName?
2. What should be done, if descriptorFile != null && (null == fileName || fileName.isEmpty())?
3. What should be done, if descriptorFile == null && null != fileName? 4a) What should be done, if descriptorFile == null && null != fileName && fileName.isEmpty()? 4b) What should be done, if descriptorFile == null && null == fileName?

to

In 1.9.0 a new method getDescrptorFile was introduced. This actually can cause various problems and has some open design questions that should be answered in this ticket. (I'll attach a branch with a test demonstrating part of the problem.)

DescriptorParser has the new method (since 1.9.0):

public Iterable<Descriptor> parseDescriptors(byte[] rawDescriptorBytes,
    File descriptorFile, String fileName);

In addition to the problem demonstrated in the (to be attached) test this raises some questions:

1. What should be done, if descriptorFile.getName() != fileName?
2. What should be done, if descriptorFile != null && (null == fileName || fileName.isEmpty())?
3. What should be done, if descriptorFile == null && null != fileName? 4a) What should be done, if descriptorFile == null && null != fileName && fileName.isEmpty()? 4b) What should be done, if descriptorFile == null && null == fileName?

The implementation of the solution should also have tests for each of the possible cases.

I don't have a good answer yet, but my hope is that we can get rid of fileName in the medium term. See also #22207 (moved). Maybe there are more descriptors that we'd have to change for this, but I really hope that all descriptors will be self-describing without relying on file names in the future.

Regarding descriptorFile, I think we'll have to permit null there, because it's not a strict requirements that descriptors come from a descriptor file. For example, an application might want to store descriptors in a database and parse them at a later time, and then it won't necessarily have the file name anymore.

I'll think more about this and maybe come up with a better answer later.

I'm not sure whether we need to do anything at all here. The JavaDocs specify what the two parameters are used for:

   * @param descriptorFile Optional descriptor file reference included in
   *     parsed/unparseable descriptors
   * @param fileName Descriptor file name used for parsing the descriptor
   *     publication time of some descriptor types

Basically, there is no formal relationship between the two parameters. In many cases fileName will be the same as descriptorFile.getName(), but that's not required. The two parameters are used for very different purposes.

And ideally we'd get rid of fileName anyway and only keep the optional descriptorFile. But that's for later.

So, I don't see a bug here (yet). I do see an interface that could be designed more clearly. But I think the best way to get there is to throw out fileName as soon as we can.

What do you think needs to be done here?

Why not throw out the File parameter? If byte arrays are supplied the filename parameter can be used for adding the 'source' to the descriptor raw bytes, which may be useful for debugging/troubleshooting and identifying weird descriptors. Or, turn filename into a sourceMarker string?

Removing this from 2.0.0, but ambiguous design is a defect. The interdependence of the two parameters is a very valid assumption.

Trac:
Milestone: metrics-lib 2.0.0 to N/A

Replying to iwakeh:

Why not throw out the File parameter?

We want the File reference to be set in every Descriptor that DescriptorParser returns. We could indeed throw out the File parameter and set it ourselves.

If byte arrays are supplied the filename parameter can be used for adding the 'source' to the descriptor raw bytes, which may be useful for debugging/troubleshooting and identifying weird descriptors.

I'd rather get rid of it.

Or, turn filename into a sourceMarker string?

The issue is that we sometimes parse parts from the filename and include it in the Descriptor. I want to change that. But not before 2.0.0 comes out. :)

Replying to iwakeh:

Removing this from 2.0.0, but ambiguous design is a defect. The interdependence of the two parameters is a very valid assumption.

I agree. I also feel we're not done yet with making interfaces self-explanatory or even unambiguous. But I also agree that it's something for post-2.0.0.

Attempt to make summary clearer.

Trac:
Summary: descriptor file processing to Resolve ambiguity between descriptorFile and fileName when parsing descriptors

Trac:
Keywords: N/A deleted, metrics-2018 added

This ticket has not been modified in the last 2 months or even longer. Setting priority to medium.

Trac:
Priority: High to Medium

Trac:
Owner: metrics-team to iwakeh
Status: new to accepted

Please review this patch, which (hopefully) clarifies the situation by a more verbose javadoc description and a parameter name change.

Trac:
Cc: N/A to metrics-team
Status: accepted to needs_review

Trac:
Milestone: N/A to metrics-lib 2.3.0

Patch looks good! Merged. Closing. Thanks!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

mentioned in issue #22983 (moved)

mentioned in issue #25523 (moved)