Bug 230183 - [remotesvcs] Remove create org.eclipse.ecf.discovery.identity.IServiceID by java.lang.String
Summary: [remotesvcs] Remove create org.eclipse.ecf.discovery.identity.IServiceID by j...
Status: RESOLVED FIXED
Alias: None
Product: ECF
Classification: RT
Component: ecf.discovery (show other bugs)
Version: unspecified   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: ecf.core-inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: plan
Depends on:
Blocks: 259263 259273
  Show dependency tree
 
Reported: 2008-05-05 07:38 EDT by Markus Kuppe CLA
Modified: 2008-12-18 11:31 EST (History)
3 users (show)

See Also:


Attachments
mylyn/context/zip (70.75 KB, application/octet-stream)
2008-12-18 10:20 EST, Markus Kuppe CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Scott Lewis CLA 2008-05-05 10:28:01 EDT
Hi Markus,

I haven't had a chance to respond on the mailing list (too busy with work considerations), but I don't believe this is a good idea.

Some specifics,

* Makes the code overly complex because of string mangling (which is also error prone)

I agree that it does make the implementation code more complex.

* Current ECF String rep is arbitrary and non-standard

But AFAIK this is because a standard for naming services/service types that does not yet exist.  I would be very happy if such a standard did exist.  In fact, I think we should consider refining our own and working with standardization bodies (e.g. OSGi EE) to standardize such naming.

* Current ECF String rep might mismatch with new providers

True...as with anything that is not standardized but must be implemented by providers.

* Creating objects by ctor/setters is more OOish and thus easier for the consumer to use.

This I'm not convinced about.  Using factories for extensible construction of objects (particularly immutable objects like IDs/ServiceIDs) is well-established and common in Eclipse and elsewhere.  

Further, the construction via factory is in keeping with construction of other IDs, and allows the IDCreationException to be thrown in response to failed parsing of Strings.

But lets keep discussing here on this bug.


Comment 2 Markus Kuppe CLA 2008-05-07 01:38:41 EDT
(In reply to comment #1)
> * Current ECF String rep is arbitrary and non-standard
> 
> But AFAIK this is because a standard for naming services/service types that
> does not yet exist.  I would be very happy if such a standard did exist.  In
> fact, I think we should consider refining our own and working with
> standardization bodies (e.g. OSGi EE) to standardize such naming.

What is the current stage in the OSGi EE in this regards?

> * Creating objects by ctor/setters is more OOish and thus easier for the
> consumer to use.
> 
> This I'm not convinced about.  Using factories for extensible construction of
> objects (particularly immutable objects like IDs/ServiceIDs) is
> well-established and common in Eclipse and elsewhere.
> 
> Further, the construction via factory is in keeping with construction of other
> IDs, 

Lets keep the factory but with different signatures, which might lead to a rather long parameter list though. But this could be solved by only offering the mandatory fields in the factory an the rest via setters (which could also throw IDCreationException).
While we're at it, what about checked vs. unchecked exceptions? Do we need the checked IDCreationException?

> and allows the IDCreationException to be thrown in response to failed
> parsing of Strings.

IDCreationException would become much less likely if not constructed by a String.
Comment 3 Scott Lewis CLA 2008-05-07 04:26:34 EDT
(In reply to comment #2)
> (In reply to comment #1)
> > * Current ECF String rep is arbitrary and non-standard
> > 
> > But AFAIK this is because a standard for naming services/service types that
> > does not yet exist.  I would be very happy if such a standard did exist.  In
> > fact, I think we should consider refining our own and working with
> > standardization bodies (e.g. OSGi EE) to standardize such naming.
> 
> What is the current stage in the OSGi EE in this regards?


All I know is the work on RFC 119(?)...the distributed services proposal.  AFAIK, it doesn't try to specify a syntax for naming of service types and services.


> 
> > * Creating objects by ctor/setters is more OOish and thus easier for the
> > consumer to use.
> > 
> > This I'm not convinced about.  Using factories for extensible construction of
> > objects (particularly immutable objects like IDs/ServiceIDs) is
> > well-established and common in Eclipse and elsewhere.
> > 
> > Further, the construction via factory is in keeping with construction of other
> > IDs, 
> 
> Lets keep the factory but with different signatures, which might lead to a
> rather long parameter list though. 


Yes.  But I don't think this would be very appealing in general...and put the burden on the client rather than the provider.   It seems to me like the burden on the provider (implementer) to create a parser is not too bad (modulo some standardization).
 
>But this could be solved by only offering
> the mandatory fields in the factory an the rest via setters (which could also
> throw IDCreationException).
> While we're at it, what about checked vs. unchecked exceptions? Do we need the
> checked IDCreationException?

It's a close call in this case IMHO...but IDCreateException could reasonably be a RuntimeException...but at least initially this didn't feel right to me (precisely because IDCreateExceptions would be thrown under some pretty normal conditions (e.g. someone passes null String into the factory).


> 
> > and allows the IDCreationException to be thrown in response to failed
> > parsing of Strings.
> 
> IDCreationException would become much less likely if not constructed by a
> String.
> 

True...but again it seems like this puts quite a bit more burden on the client...agree?

Comment 4 Markus Kuppe CLA 2008-05-07 05:15:49 EDT
(In reply to comment #3)
> All I know is the work on RFC 119(?)...the distributed services proposal. 
> AFAIK, it doesn't try to specify a syntax for naming of service types and
> services.

So what are our options here? Is this in the scope of RFC 119 anyway?

> Yes.  But I don't think this would be very appealing in general...and put the
> burden on the client rather than the provider.   It seems to me like the burden
> on the provider (implementer) to create a parser is not too bad (modulo some
> standardization).

I guess it mostly depends on the usage scenario of the client. If the client simply needs to pass in a input string from an UI component directly, a factory taking a string is definitely better. But if it's to be constructed programmatically, using setters might be more appropriate to use.

> It's a close call in this case IMHO...but IDCreateException could reasonably be
> a RuntimeException...but at least initially this didn't feel right to me
> (precisely because IDCreateExceptions would be thrown under some pretty normal
> conditions (e.g. someone passes null String into the factory).

It should be consistent with the rest of ECF. If it makes good sense to model it as a checked exception there, discovery should follow.

Comment 5 Markus Kuppe CLA 2008-05-07 05:16:20 EDT
Adding Jan
Comment 6 Markus Kuppe CLA 2008-12-18 10:20:00 EST
Instead of removing String ctors, I've added in org.eclipse.ecf.discovery.identity.IServiceIDFactory

	/**
	 * Create an IServiceID.  Creates an immutable IServiceID with a non-<code>null</code> {@link IServiceTypeID}
	 * and a potentially <code>null</code> service name. 
	 * 
	 * @param namespace the Namespace instance to create the service ID with.  Must not be <code>null</code>.
	 * @param services Array containing the ordered naming hierarchy from 0...n. Must not be <code>null</code>.
	 * @param scopes Array containing all scopes or {@link IServiceTypeID#DEFAULT_SCOPE} for default. Must not be <code>null</code>.
	 * @param protocols Array containing all protocols or {@link IServiceTypeID#DEFAULT_PROTO} for default. Must not be <code>null</code>.
	 * @param namingAuthority the NamingAuthority or {@link IServiceTypeID#DEFAULT_NA} for default. Must not be <code>null</code>.
	 * @param serviceName the service name for the service ID.  May be <code>null</code>.
	 *
	 * @since 3.0
	 *
	 * @return IServiceID created.  Will not be <code>null</code>.
	 * @throws IDCreateException if some problem creating the new IServiceID.
	 */
	public IServiceID createServiceID(Namespace namespace, String[] services, String[] scopes, String[] protocols, String namingAuthority, String serviceName) throws IDCreateException;

	/**
	 * Create an IServiceID.  Creates an immutable IServiceID with a non-<code>null</code> {@link IServiceTypeID}
	 * and a potentially <code>null</code> service name. Scope, Protocol and NamingAuthority will be set to
	 * {@link IServiceTypeID#DEFAULT_SCOPE}, {@link IServiceTypeID#DEFAULT_PROTO}, {@link IServiceTypeID#DEFAULT_NA}
	 * @param namespace the Namespace instance to create the service ID with.  Must not be <code>null</code>.
	 * @param services Array containing the ordered naming hierarchy from 0...n. Must not be <code>null</code>.
	 * @param serviceName the service name for the service ID.  May be <code>null</code>.
	 *
	 * @since 3.0
	 *
	 * @return IServiceID created.  Will not be <code>null</code>.
	 * @throws IDCreateException if some problem creating the new IServiceID.
	 */
	public IServiceID createServiceID(Namespace namespace, String[] services, String serviceName) throws IDCreateException;
Comment 7 Markus Kuppe CLA 2008-12-18 10:20:08 EST
Created attachment 120840 [details]
mylyn/context/zip