Bug in XYSeriesCollection

[nike]

Hey,
I'm running JFreeChart 1.0.14 ( and JCommon 1.0.17).

I have an issue with the XYSeriesCollection when trying to change the key of a series.

The methode getSeries(Comparable key) throws an UnknownKeyException if key is not found in the collection.

Code: Select all

public XYSeries getSeries(Comparable key) {
        if (key == null) {
            throw new IllegalArgumentException("Null 'key' argument.");
        }
        Iterator iterator = this.data.iterator();
        while (iterator.hasNext()) {
            XYSeries series = (XYSeries) iterator.next();
            if (key.equals(series.getKey())) {
                return series;
            }
        }
        throw new UnknownKeyException("Key not found: " + key);
    }

By trying to change the key of a series in the XYSeriesCollection, the vetoableChange method is called. This produces an error and I consider it to be a bug:

Code: Select all

public void vetoableChange(PropertyChangeEvent e) throws PropertyVetoException {
        // if it is not the series name, then we have no interest
        if (!"Key".equals(e.getPropertyName())) {
            return;
        }
        
        // to be defensive, let's check that the source series does in fact
        // belong to this collection
        Series s = (Series) e.getSource();
        if (getSeries(s.getKey()) == null) {
            throw new IllegalStateException("Receiving events from a series " +
                    "that does not belong to this collection.");
        }
        // check if the new series name already exists for another series
        Comparable key = (Comparable) e.getNewValue();
        if (this.getSeries(key) != null) {
            throw new PropertyVetoException("Duplicate key2", e);
        }
}

So, in the last section the method is checking for a null return in this.getSeries(key), that will never appear. This makes it impossible to change the key of a series.
I'm using a corrected version of the XYSeriesCollection at the moment, where getSeries(Comparable key) returns null if a series does not exist.

Is this a bug?
Thanks,
Nik

[Magnar]

I had the same problem. For those interested changing vetoableChange in XYSeriesCollection to the following also does the trick.

Code: Select all

public void vetoableChange(PropertyChangeEvent e)
			throws PropertyVetoException {
		// if it is not the series name, then we have no interest
		if (!"Key".equals(e.getPropertyName())) {
			return;
		}

		// to be defensive, let's check that the source series does in fact
		// belong to this collection
		Series s = (Series) e.getSource();
		if (getSeries(s.getKey()) == null) {
			throw new IllegalStateException("Receiving events from a series "
					+ "that does not belong to this collection.");
		}
		// check if the new series name already exists for another series
		Comparable key = (Comparable) e.getNewValue();

		XYSeries k = null;
		try {
			k = this.getSeries(key);
		} catch (Exception e1) {
			// ignore
		}

		if (k != null) {
			throw new PropertyVetoException("Duplicate key2", e);
		}
	}

[brimborium]

I would not recommend catching Exception. Always specify which Exception. In this case

Code: Select all

XYSeries k = null;
try {
   k = this.getSeries(key);
} catch (Exception e1) {
   // ignore
}

would become

Code: Select all

XYSeries k = null;
try {
   k = this.getSeries(key);
} catch (IllegalArgumentException e1) {
   // handle illegal argument exception
} catch (UnknownKeyException e2) {
   // ignore
}

[david.gilbert]

Yes, this is a bug - thanks for reporting it. I wrote this JUnit test which I think covers the case:

Code: Select all

    /**
     * Test that a series belonging to a collection can be renamed (in fact, 
     * because of a bug this was not possible in JFreeChart 1.0.14).
     */
    public void testSeriesRename() {
        // first check that a valid renaming works
        XYSeries series1 = new XYSeries("A");
        XYSeries series2 = new XYSeries("B");
        XYSeriesCollection collection = new XYSeriesCollection();
        collection.addSeries(series1);
        collection.addSeries(series2);
        series1.setKey("C");
        assertEquals("C", collection.getSeries(0).getKey());
        
        // next, check that setting a duplicate key fails
        try {
            series2.setKey("C");
        }
        catch (IllegalArgumentException e) {
            // expected
        }
        assertEquals("B", series2.getKey());  // the series name should not 
        // change because "C" is already the key for the other series in the
        // collection
    }

I will commit a fix for the upcoming 1.0.15 release (I think the easiest fix will be to use the getSeriesIndex(Comparable) method which returns -1 rather than throwing an exception when the series key is not found).