Request for Review: Attributes.map generic types
Remi Forax
forax at univ-mlv.fr
Tue Oct 10 22:24:03 UTC 2017
Hi Philipp,
a simple code like this will break !
Attributes attrs = ...
Map<Object,Object> map = attrs;
And to answer to your question, no ! type parameters were not introduced automatically but carefully by hand in order to be backward compatible.
Attributes was introduced before 1.5 so people wrote codes putting any attributes dynamically into the map, by example a pair of String. So you can not change the type arguments without breaking a lot of codes.
Exposing Attributes as a Map or declaring the field 'map' as protected were mistakes because it shows too many details of the implementation. (java.util.Properties was retrofitted the same way for the same reason).
regards,
Rémi
----- Mail original -----
> De: "Sean Mullan" <sean.mullan at oracle.com>
> À: "Philipp Kunz" <philipp.kunz at paratix.ch>, security-dev at openjdk.java.net
> Cc: "core-libs-dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Mardi 10 Octobre 2017 22:30:48
> Objet: Re: Request for Review: Attributes.map generic types
> Cross-posting to core-libs-dev as this proposal is really more
> appropriate for review on that list.
>
> --Sean
>
> On 10/3/17 1:48 AM, Philipp Kunz wrote:
>> Hi
>>
>> This has not been asked for and there is also no bug yet but
>> nevertheless let me propose to change Attributes.map to specific generic
>> types. It looks like the type parameters were added automatically with
>> their introduction in java 5. I figure that no specific test is required
>> in this particular case because it's a pure compile-time issue which is
>> already sufficiently covered with existing code and there are also tests
>> that already now involve manifests with attributes at run-time.
>>
>> Any feedback or thoughts? Would someone volunteer to sponsor it?
>>
>> Regards,
>> Philipp
>>
>>
>>
>> ----- BEGIN PATCH -----
>> diff -r 2e947e1bd907
>> src/java.base/share/classes/java/util/jar/Attributes.java
>> --- a/src/java.base/share/classes/java/util/jar/Attributes.java Mon Oct
>> 02 10:04:22 2017 -0700
>> +++ b/src/java.base/share/classes/java/util/jar/Attributes.java Tue Oct
>> 03 07:41:40 2017 +0200
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights
>> reserved.
>> + * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights
>> reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> @@ -54,11 +54,11 @@
>> * @see Manifest
>> * @since 1.2
>> */
>> -public class Attributes implements Map<Object,Object>, Cloneable {
>> +public class Attributes implements Map<Attributes.Name, String>,
>> Cloneable {
>> /**
>> * The attribute name-value mappings.
>> */
>> - protected Map<Object,Object> map;
>> + protected Map<Name, String> map;
>>
>> /**
>> * Constructs a new, empty Attributes object with default size.
>> @@ -87,7 +87,6 @@
>> map = new LinkedHashMap<>(attr);
>> }
>>
>> -
>> /**
>> * Returns the value of the specified attribute name, or null if the
>> * attribute name was not found.
>> @@ -96,7 +95,7 @@
>> * @return the value of the specified attribute name, or null if
>> * not found.
>> */
>> - public Object get(Object name) {
>> + public String get(Object name) {
>> return map.get(name);
>> }
>>
>> @@ -116,7 +115,7 @@
>> * @throws IllegalArgumentException if the attribute name is invalid
>> */
>> public String getValue(String name) {
>> - return (String)get(new Attributes.Name(name));
>> + return get(new Attributes.Name(name));
>> }
>>
>> /**
>> @@ -133,7 +132,7 @@
>> * not found.
>> */
>> public String getValue(Name name) {
>> - return (String)get(name);
>> + return get(name);
>> }
>>
>> /**
>> @@ -144,11 +143,9 @@
>> * @param name the attribute name
>> * @param value the attribute value
>> * @return the previous value of the attribute, or null if none
>> - * @exception ClassCastException if the name is not a Attributes.Name
>> - * or the value is not a String
>> */
>> - public Object put(Object name, Object value) {
>> - return map.put((Attributes.Name)name, (String)value);
>> + public String put(Attributes.Name name, String value) {
>> + return map.put(name, value);
>> }
>>
>> /**
>> @@ -168,7 +165,7 @@
>> * @exception IllegalArgumentException if the attribute name is
>> invalid
>> */
>> public String putValue(String name, String value) {
>> - return (String)put(new Name(name), value);
>> + return put(new Name(name), value);
>> }
>>
>> /**
>> @@ -178,7 +175,7 @@
>> * @param name attribute name
>> * @return the previous value of the attribute, or null if none
>> */
>> - public Object remove(Object name) {
>> + public String remove(Object name) {
>> return map.remove(name);
>> }
>>
>> @@ -208,15 +205,14 @@
>> * Copies all of the attribute name-value mappings from the specified
>> * Attributes to this Map. Duplicate mappings will be replaced.
>> *
>> - * @param attr the Attributes to be stored in this map
>> - * @exception ClassCastException if attr is not an Attributes
>> + * @param attr the attributes to be stored in this map, may also be of
>> + * type {@link Attributes}
>> */
>> - public void putAll(Map<?,?> attr) {
>> - // ## javac bug?
>> - if (!Attributes.class.isInstance(attr))
>> - throw new ClassCastException();
>> - for (Map.Entry<?,?> me : (attr).entrySet())
>> + public void putAll(Map<? extends Name, ? extends String> attr) {
>> + for (Map.Entry<? extends Name, ? extends String> me
>> + : attr.entrySet()) {
>> put(me.getKey(), me.getValue());
>> + }
>> }
>>
>> /**
>> @@ -243,14 +239,14 @@
>> /**
>> * Returns a Set view of the attribute names (keys) contained in
>> this Map.
>> */
>> - public Set<Object> keySet() {
>> + public Set<Name> keySet() {
>> return map.keySet();
>> }
>>
>> /**
>> * Returns a Collection view of the attribute values contained in
>> this Map.
>> */
>> - public Collection<Object> values() {
>> + public Collection<String> values() {
>> return map.values();
>> }
>>
>> @@ -258,7 +254,7 @@
>> * Returns a Collection view of the attribute name-value mappings
>> * contained in this Map.
>> */
>> - public Set<Map.Entry<Object,Object>> entrySet() {
>> + public Set<Map.Entry<Name, String>> entrySet() {
>> return map.entrySet();
>> }
>>
>> @@ -290,7 +286,7 @@
>> * the Attributes returned can be safely modified without affecting
>> * the original.
>> */
>> - public Object clone() {
>> + public Attributes clone() {
>> return new Attributes(this);
>> }
>>
>> @@ -300,12 +296,12 @@
>> */
>> @SuppressWarnings("deprecation")
>> void write(DataOutputStream os) throws IOException {
>> - for (Entry<Object, Object> e : entrySet()) {
>> + for (Entry<Name, String> e : entrySet()) {
>> StringBuffer buffer = new StringBuffer(
>> - ((Name) e.getKey()).toString());
>> + e.getKey().toString());
>> buffer.append(": ");
>>
>> - String value = (String) e.getValue();
>> + String value = e.getValue();
>> if (value != null) {
>> byte[] vb = value.getBytes("UTF8");
>> value = new String(vb, 0, 0, vb.length);
>> @@ -343,14 +339,14 @@
>>
>> // write out all attributes except for the version
>> // we wrote out earlier
>> - for (Entry<Object, Object> e : entrySet()) {
>> - String name = ((Name) e.getKey()).toString();
>> + for (Entry<Name, String> e : entrySet()) {
>> + String name = e.getKey().toString();
>> if ((version != null) && !(name.equalsIgnoreCase(vername))) {
>>
>> StringBuffer buffer = new StringBuffer(name);
>> buffer.append(": ");
>>
>> - String value = (String) e.getValue();
>> + String value = e.getValue();
>> if (value != null) {
>> byte[] vb = value.getBytes("UTF8");
>> value = new String(vb, 0, 0, vb.length);
>> diff -r 2e947e1bd907
>> src/java.base/share/classes/sun/net/www/protocol/jar/URLJarFile.java
>> ---
>> a/src/java.base/share/classes/sun/net/www/protocol/jar/URLJarFile.java
>> Mon Oct 02 10:04:22 2017 -0700
>> +++
>> b/src/java.base/share/classes/sun/net/www/protocol/jar/URLJarFile.java
>> Tue Oct 03 07:41:40 2017 +0200
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2001, 2016, Oracle and/or its affiliates. All rights
>> reserved.
>> + * Copyright (c) 2001, 2017, Oracle and/or its affiliates. All rights
>> reserved.
>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> *
>> * This code is free software; you can redistribute it and/or modify it
>> @@ -148,14 +148,14 @@
>>
>> Manifest man = new Manifest();
>> Attributes attr = man.getMainAttributes();
>> - attr.putAll((Map)superAttr.clone());
>> + attr.putAll(superAttr.clone());
>>
>> // now deep copy the manifest entries
>> if (superEntries != null) {
>> Map<String, Attributes> entries = man.getEntries();
>> for (String key : superEntries.keySet()) {
>> Attributes at = superEntries.get(key);
>> - entries.put(key, (Attributes) at.clone());
>> + entries.put(key, at.clone());
>> }
>> }
>>
>> @@ -261,7 +261,7 @@
>> if (e != null) {
>> Attributes a = e.get(getName());
>> if (a != null)
>> - return (Attributes)a.clone();
>> + return a.clone();
>> }
>> }
>> return null;
>> diff -r 2e947e1bd907
>> src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
>> ---
>> a/src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
>> Mon Oct 02 10:04:22 2017 -0700
>> +++
>> b/src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
>> Tue Oct 03 07:41:40 2017 +0200
>> @@ -30,6 +30,7 @@
>> import java.security.CodeSigner;
>> import java.util.*;
>> import java.util.jar.*;
>> +import java.util.jar.Attributes.Name;
>>
>> import java.util.Base64;
>>
>> @@ -122,7 +123,7 @@
>> }
>> }
>>
>> - for (Map.Entry<Object,Object> se : attr.entrySet()) {
>> + for (Map.Entry<Name, String> se : attr.entrySet()) {
>> String key = se.getKey().toString();
>>
>> if (key.toUpperCase(Locale.ENGLISH).endsWith("-DIGEST")) {
>> @@ -146,7 +147,7 @@
>> digest.reset();
>> digests.add(digest);
>> manifestHashes.add(
>> - Base64.getMimeDecoder().decode((String)se.getValue()));
>> + Base64.getMimeDecoder().decode(se.getValue()));
>> }
>> }
>> }
>> diff -r 2e947e1bd907
>> src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java
>> ---
>> a/src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java
>> Mon Oct 02 10:04:22 2017 -0700
>> +++
>> b/src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java
>> Tue Oct 03 07:41:40 2017 +0200
>> @@ -46,6 +46,7 @@
>> import java.util.Locale;
>> import java.util.Map;
>> import java.util.jar.Attributes;
>> +import java.util.jar.Attributes.Name;
>> import java.util.jar.JarException;
>> import java.util.jar.JarFile;
>> import java.util.jar.Manifest;
>> @@ -445,7 +446,7 @@
>> boolean validEntry = false;
>>
>> // go through all the attributes and process *-Digest-Manifest
>> entries
>> - for (Map.Entry<Object,Object> se : mattr.entrySet()) {
>> + for (Map.Entry<Name, String> se : mattr.entrySet()) {
>>
>> String key = se.getKey().toString();
>>
>> @@ -469,7 +470,7 @@
>> if (digest != null) {
>> byte[] computedHash = md.manifestDigest(digest);
>> byte[] expectedHash =
>> - Base64.getMimeDecoder().decode((String)se.getValue());
>> + Base64.getMimeDecoder().decode(se.getValue());
>>
>> if (debug != null) {
>> debug.println("Signature File: Manifest digest " +
>> @@ -517,7 +518,7 @@
>>
>> // go through all the attributes and process
>> // digest entries for the manifest main attributes
>> - for (Map.Entry<Object,Object> se : mattr.entrySet()) {
>> + for (Map.Entry<Name, String> se : mattr.entrySet()) {
>> String key = se.getKey().toString();
>>
>> if (key.toUpperCase(Locale.ENGLISH).endsWith(ATTR_DIGEST)) {
>> @@ -540,7 +541,7 @@
>> md.get(ManifestDigester.MF_MAIN_ATTRS, false);
>> byte[] computedHash = mde.digest(digest);
>> byte[] expectedHash =
>> - Base64.getMimeDecoder().decode((String)se.getValue());
>> + Base64.getMimeDecoder().decode(se.getValue());
>>
>> if (debug != null) {
>> debug.println("Signature File: " +
>> @@ -620,7 +621,7 @@
>> //hex.encodeBuffer(data, System.out);
>>
>> // go through all the attributes and process *-Digest entries
>> - for (Map.Entry<Object,Object> se : sfAttr.entrySet()) {
>> + for (Map.Entry<Name, String> se : sfAttr.entrySet()) {
>> String key = se.getKey().toString();
>>
>> if (key.toUpperCase(Locale.ENGLISH).endsWith("-DIGEST")) {
>> @@ -643,7 +644,7 @@
>> boolean ok = false;
>>
>> byte[] expected =
>> - Base64.getMimeDecoder().decode((String)se.getValue());
>> + Base64.getMimeDecoder().decode(se.getValue());
>> byte[] computed;
>> if (workaround) {
>> computed = mde.digestWorkaround(digest);
>> diff -r 2e947e1bd907
>> src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
>> ---
>> a/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
>> Mon Oct 02 10:04:22 2017 -0700
>> +++
>> b/src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
>> Tue Oct 03 07:41:40 2017 +0200
>> @@ -685,7 +685,7 @@
>> // Manifest exists. Read its raw bytes.
>> mfRawBytes = zipFile.getInputStream(mfFile).readAllBytes();
>> manifest.read(new ByteArrayInputStream(mfRawBytes));
>> - oldAttr = (Attributes) (manifest.getMainAttributes().clone());
>> + oldAttr = manifest.getMainAttributes().clone();
>> } else {
>> // Create new manifest
>> Attributes mattr = manifest.getMainAttributes();
>> ----- END PATCH -----
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>>
>> Paratix GmbH
>> St Peterhofstatt 11
>> 8001 Zürich
>>
>> +41 (0)76 397 79 35
> > philipp.kunz at paratix.ch <mailto:philipp.kunz at paratix.ch>
More information about the core-libs-dev
mailing list