Thread: Re: [Ogoglio-developers] [Ogoglio-commits] SF.net SVN: ogoglio: [224] spaces/trunk/src/com/ogoglio

Hi Trevor,

Thanks for adding the patches... I still need to figure out what goes where
:) and how.

In regards to using log4j I believe it to be a better solution than using
System.out/err, since we can benefit from the logging levels, changing
logging level at runtime, code guards etc.

Just as an example, "System.out.println(new Date() + ": Deleted space: " +
spaceID);"
Why would I want to have this line in the logs when the system is stable? It
can help me debugging the system but not more than that. If we used log4j I
might have marked this message as info level and usually I would run the
system at warn level...

In regards to the space delete issue, I think it make more sense to put the
DELETE in the AccountServlet and not in the SpaceServlet. The reason is that
since spaces are created in the AccountServlet it would make more sense to
delete it in the same place i.e. AccountServlet. Also, since SpaceServlet
manages the space's inner data only, I think it is better that the creation
and deletion of space stays in the level of the account.

Gal.

On 7/10/07, tre...@us... <
tre...@us...> wrote:
>
> Revision: 224
>           http://ogoglio.svn.sourceforge.net/ogoglio/?rev=224&view=rev
> Author:   trevorolio
> Date:     2007-07-10 13:03:06 -0700 (Tue, 10 Jul 2007)
>
> Log Message:
> -----------
> Implemented space deletion, based in part on Gal's patch.
>
> Modified Paths:
> --------------
>     spaces/trunk/src/com/ogoglio/client/ClientTests.java
>     spaces/trunk/src/com/ogoglio/client/WebAPIClient.java
>     spaces/trunk/src/com/ogoglio/persist/SpacePersistTasks.java
>     spaces/trunk/src/com/ogoglio/sim/Sim.java
>     spaces/trunk/src/com/ogoglio/sim/SpaceSimulator.java
>     spaces/trunk/src/com/ogoglio/sim/site/SimServlet.java
>     spaces/trunk/src/com/ogoglio/site/AccountServlet.java
>     spaces/trunk/src/com/ogoglio/site/SpaceServlet.java
>
> Modified: spaces/trunk/src/com/ogoglio/client/ClientTests.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/client/ClientTests.java        2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/client/ClientTests.java        2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -374,10 +374,6 @@
>              thingDocs = webClient1.getThingDocuments();
>              assertEquals(0, thingDocs.length);
>
> -            webClient1.deletePossession(possDoc.getPossessionID());
> -            possDocs = webClient1.getPossessionDocuments(USERNAME1);
> -            assertEquals(numPossessions, possDocs.length);
> -
>              String guestCookie1 = WebAPIClient.requestGuestCookie
> (serviceURI1);
>              assertNotNull(guestCookie1);
>              try {
> @@ -402,6 +398,22 @@
>              assertEquals(2, userDocs.length);
>              assertTrue(guestCookie1.equals(userDocs[1].getUsername()) ||
> guestCookie1.equals(userDocs[0].getUsername()));
>
> +            possDoc = webClient1.addPossessionToSpace(possDocs[numPossessions].getPossessionID(),
> spaceDocument.getSpaceID());
> +            assertNotNull(possDoc);
> +            try {
> +                Thread.sleep(100);
> +            } catch (Exception e) {
> +            }
> +            thingDocs = webClient1.getThingDocuments();
> +            assertEquals(1, thingDocs.length);
> +            assertTrue(webClient1.deleteSpace());
> +            possDoc = webClient1.getPossessionDocuments(
> possDoc.getOwnerUsername())[0];
> +            assertEquals(-1, possDoc.getSpaceID());
> +            assertEquals(-1, possDoc.getThingID());
> +
> +            webClient1.deletePossession(possDoc.getPossessionID());
> +            possDocs = webClient1.getPossessionDocuments(USERNAME1);
> +            assertEquals(numPossessions, possDocs.length);
>          } catch (IOException e) {
>              e.printStackTrace();
>              fail();
>
> Modified: spaces/trunk/src/com/ogoglio/client/WebAPIClient.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/client/WebAPIClient.java       2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/client/WebAPIClient.java       2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -118,6 +118,10 @@
>          return new SpaceDocument(result);
>      }
>
> +    public boolean deleteSpace() throws IOException {
> +        return sendDelete(getSpaceURI(), authCookie);
> +    }
> +
>      public static AccountDocument updateAccount(URI serviceURI, String
> authCookie, AccountDocument accountDoc) throws IOException {
>          XMLElement result = sendAuthenticatedXML(WebAPIUtil.appendToURI(serviceURI,
> "account/" + accountDoc.getUsername()), accountDoc.toElement().toString(),
> "POST", authCookie);
>          if (result == null) {
>
> Modified: spaces/trunk/src/com/ogoglio/persist/SpacePersistTasks.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/persist/SpacePersistTasks.java 2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/persist/SpacePersistTasks.java 2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -56,16 +56,16 @@
>                      record.setPublished(spaceDocument.isPublished());
>                  }
>
> -                if(spaceDocument.getDisplaySea() != record.getDisplaySea())
> {
> +                if (spaceDocument.getDisplaySea() != record.getDisplaySea())
> {
>                      dirty = true;
>                      record.setDisplaySea(spaceDocument.getDisplaySea());
>                  }
> -
> +
>                  if (Math.abs(spaceDocument.getSeaLevel() -
> record.getSeaLevel()) > 0.0001) {
>                      dirty = true;
>                      record.setSeaLevel(spaceDocument.getSeaLevel());
>                  }
> -
> +
>                  if (dirty) {
>                      hibernateSession.update(record);
>                  }
> @@ -165,6 +165,26 @@
>          return (SpaceRecord) task.execute();
>      }
>
> +    public static boolean deleteSpace(final SpaceRecord record,
> SessionFactory sessionFactory) throws PersistException {
> +        HibernateTask task = new HibernateTask() {
> +            public Object run(Session session) throws PersistException {
> +                Query possQuery = session.getNamedQuery(
> PossessionPersistTasks.POSSESSIONS_BY_SPACE_ID);
> +                possQuery.setLong("spaceID", record.getSpaceID());
> +                PossessionRecord[] possessionRecords =
> (PossessionRecord[])possQuery.list().toArray(new PossessionRecord[0]);
> +                for (int i = 0; i < possessionRecords.length; i++) {
> +                    possessionRecords[i].setSpaceID(-1);
> +                    possessionRecords[i].setThingID(-1);
> +                    session.update(possessionRecords[i]);
> +                }
> +                session.delete(record);
> +                return Boolean.TRUE;
> +            }
> +        };
> +
> +        task.setSessionFactory(sessionFactory);
> +        return Boolean.TRUE == task.execute();
> +    }
> +
>      public static boolean delete(final SpaceMemberRecord memberRec,
> SessionFactory sessionFactory) throws PersistException {
>          HibernateTask task = new HibernateTask() {
>              public Object run(Session hibernateSession) {
> @@ -195,10 +215,10 @@
>                          return Boolean.TRUE;
>                      }
>
> -                    if(!space.isPublished()) {
> +                    if (!space.isPublished()) {
>                          return Boolean.FALSE;
>                      }
> -
> +
>                      Query membersQuery = hibernateSession.getNamedQuery(
> SpaceMemberPersistTasks.SPACE_MEMBERS_BY_SPACE_ID);
>                      membersQuery.setLong("spaceID", spaceID);
>                      SpaceMemberRecord[] members = (SpaceMemberRecord[])
> membersQuery.list().toArray(new SpaceMemberRecord[0]);
> @@ -210,7 +230,7 @@
>                      }
>                  }
>
> -                return space.isPublished() && space.getMaxGuests() > 0 ?
> Boolean.TRUE: Boolean.FALSE;
> +                return space.isPublished() && space.getMaxGuests() > 0 ?
> Boolean.TRUE : Boolean.FALSE;
>              }
>          };
>          task.setSessionFactory(sessionFactory);
>
> Modified: spaces/trunk/src/com/ogoglio/sim/Sim.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/sim/Sim.java   2007-07-10 18:13:02 UTC
> (rev 223)
> +++ spaces/trunk/src/com/ogoglio/sim/Sim.java   2007-07-10 20:03:06 UTC
> (rev 224)
> @@ -22,6 +22,8 @@
>
> import nanoxml.XMLElement;
>
> +import org.apache.commons.logging.Log;
> +import org.apache.commons.logging.LogFactory;
> import org.hibernate.SessionFactory;
>
> import com.ogoglio.media.MediaService;
> @@ -40,7 +42,6 @@
> import com.ogoglio.xml.TemplateDocument;
>
> public class Sim {
> -
>      public static final long MAX_GEOMETRY_SIZE = 1048576;
>
>      public static final long MAX_GEOMETRY_RESOURCE_SIZE = 1048576;
> @@ -165,7 +166,10 @@
>
>          public void requestSave(SpaceSimulator simulator) {
>              try {
> -                saveSpaceDocument(simulator.toSpaceDocument());
> +                if (!simulator.getDeleted()) {
> +                    saveSpaceDocument(simulator.toSpaceDocument());
> +                    return;
> +                }
>              } catch (IOException e) {
>                  e.printStackTrace();
>              }
> @@ -205,7 +209,8 @@
>          }
>      }
>
> -    private boolean shutdownSpaceSim(long spaceID) {
> +    // TODO is there a better way to shutdown this sim ?
> +    public boolean shutdownSpaceSim(long spaceID) {
>          try {
>              SpaceRecord record = SpacePersistTasks.findSpaceBySpaceID(spaceID,
> sessionFactory);
>              if (record == null) {
> @@ -219,6 +224,7 @@
>                  }
>                  return false;
>              }
> +
>              record.setSimID(-1);
>              SpacePersistTasks.update(record, sessionFactory);
>              SpaceSimulator spaceSim = null;
> @@ -245,31 +251,32 @@
>      }
>
>      public SpaceSimulator getOrCreateSpaceSimulator(SpaceRecord record) {
> -        SpaceSimulator simulator = null;
>          synchronized (spaceSimulators) {
> -            simulator = (SpaceSimulator) spaceSimulators.getForward(new
> Long(record.getSpaceID()));
> -            if (simulator == null) {
> -                SpaceDocument spaceDoc = null;
> -                try {
> -                    spaceDoc = getSpaceDocument(record.getSpaceID());
> -                } catch (IOException e) {
> -                }
> -                if (spaceDoc == null) {
> -                    spaceDoc = new SpaceDocument(record);
> -                } else {
> -                    spaceDoc.setDisplayName(record.getDisplayName());
> -                    spaceDoc.setMaxGuests(record.getMaxGuests());
> -                    spaceDoc.setSimID(record.getSimID());
> -                    spaceDoc.setPublished(record.isPublished());
> -                }
> +            SpaceSimulator simulator = (SpaceSimulator)
> spaceSimulators.getForward(new Long(record.getSpaceID()));
> +            if (simulator != null) {
> +                return simulator;
> +            }
>
> -                simulator = new SpaceSimulator(spaceDoc,
> spaceSimulatorListener);
> -                spaceSimulators.put(new Long(record.getSpaceID()),
> simulator);
> -                simulator.startSim();
> -                System.out.println(new Date() + ": Starting space sim " +
> record.getSpaceID() + ": " + record.getDisplayName());
> +            SpaceDocument spaceDoc = null;
> +            try {
> +                spaceDoc = getSpaceDocument(record.getSpaceID());
> +            } catch (IOException e) {
>              }
> +            if (spaceDoc == null) {
> +                spaceDoc = new SpaceDocument(record);
> +            } else {
> +                spaceDoc.setDisplayName(record.getDisplayName());
> +                spaceDoc.setMaxGuests(record.getMaxGuests());
> +                spaceDoc.setSimID(record.getSimID());
> +                spaceDoc.setPublished(record.isPublished());
> +            }
> +
> +            simulator = new SpaceSimulator(spaceDoc,
> spaceSimulatorListener);
> +            spaceSimulators.put(new Long(record.getSpaceID()),
> simulator);
> +            simulator.startSim();
> +            System.out.println(new Date() + ": Starting space sim " +
> record.getSpaceID() + ": " + record.getDisplayName());
> +            return simulator;
>          }
> -        return simulator;
>      }
>
>      private void saveSpaceDocument(SpaceDocument doc) throws IOException
> {
>
> Modified: spaces/trunk/src/com/ogoglio/sim/SpaceSimulator.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/sim/SpaceSimulator.java        2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/sim/SpaceSimulator.java        2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -26,6 +26,9 @@
> import javax.vecmath.Point3d;
> import javax.vecmath.Vector3d;
>
> +import org.apache.commons.logging.Log;
> +import org.apache.commons.logging.LogFactory;
> +
> import com.ogoglio.client.UserInputListener;
> import com.ogoglio.client.model.Door;
> import com.ogoglio.client.model.GeometryProvider;
> @@ -64,7 +67,7 @@
>      private SpaceScriptEngine scriptEngine = null;
>
>      private long lastLogoutTime = System.currentTimeMillis();
> -
> +
>      //TODO stop shoving the log into memory
>      private Vector log = new Vector();
>
> @@ -73,7 +76,10 @@
>      private J3DRenderer renderer = null;
>
>      private HashMap settings = new HashMap();
> -
> +
> +    // a flag that mark this simulator to be deleted after it will be
> shutdown
> +    private boolean deleted = false;
> +
>      public SpaceSimulator(SpaceDocument spaceDocument, Listener listener)
> {
>          ArgumentUtils.assertNotNull(listener);
>          this.listener = listener;
> @@ -99,14 +105,14 @@
>          for (int i = 0; i < settingDocs.length; i++) {
>              settings.put(settingDocs[i].getKey(),
> settingDocs[i].getValue());
>          }
> -
> +
>          DoorDocument[] doorDocs = spaceDocument.getDoorDocuments();
>          for (int i = 0; i < doorDocs.length; i++) {
>              Template template = new Template(listener.getTemplateDocument
> (doorDocs[i].getTemplateID()));
>              space.addTemplate(template);
>              space.addDoor(new Door(space, template, doorDocs[i]));
>          }
> -
> +
>          scriptEngine = new SpaceScriptEngine(this);
>          //TODO construct the space script
>      }
> @@ -114,12 +120,12 @@
>      public Listener getListener() {
>          return listener;
>      }
> -
> +
>      public interface Listener {
>          public void generatedSpaceEvent(SpaceEvent event, SpaceSimulator
> spaceSimulator);
>
>          public void generatedSpaceEventForUser(String username,
> SpaceEvent event, SpaceSimulator spaceSimulator);
> -
> +
>          public TemplateDocument getTemplateDocument(long templateID);
>
>          public String getTemplateScript(long templateID);
> @@ -198,12 +204,14 @@
>              if (cleaned) {
>                  return;
>              }
> -            System.out.println(new Date() + ": Stopping " +
> space.getSpaceID() + ": " + space.getDisplayName());
> +            System.out.println(new Date() + ": Stopping" + (deleted ? "
> deleted" : "") + " space " + space.getSpaceID() + ": " +
> space.getDisplayName());
>              cleaned = true;
>              renderer.stopRenderer();
>              scriptEngine.cleanup();
>              simThread.queue.close();
> -            listener.requestSave(this);
> +            if (!deleted) {
> +                listener.requestSave(this);
> +            }
>          } catch (Throwable e) {
>              e.printStackTrace();
>          }
> @@ -224,7 +232,7 @@
>              super("SimThread");
>              setDaemon(true);
>          }
> -
> +
>          public void run() {
>              while (!cleaned) {
>                  try {
> @@ -280,7 +288,7 @@
>
>                      } else if (SpaceEvent.SHAPE_START_MOTION_EVENT.equals
> (event.getName())) {
>                          String shapeName = event.getStringProperty(
> SpaceEvent.SHAPE_NAME);
> -                        if(shapeName == null) {
> +                        if (shapeName == null) {
>                              log("Tried to move a shape with no name " +
> event);
>                              continue;
>                          }
> @@ -300,7 +308,7 @@
>
>                      } else if (SpaceEvent.SHAPE_STOP_MOTION_EVENT.equals(
> event.getName())) {
>                          String shapeName = event.getStringProperty(
> SpaceEvent.SHAPE_NAME);
> -                        if(shapeName == null) {
> +                        if (shapeName == null) {
>                              log("Tried to stop a shape with no name " +
> event);
>                              continue;
>                          }
> @@ -362,23 +370,23 @@
>                          }
>
>                          String chatMessage = event.getStringProperty(
> SpaceEvent.TSE_MESSAGE);
> -
> +
>                          // this is a hack to play pre-baked animations in
> demos
>                          // TODO create a user animation record and a
> method to dl and play them
> -                        if("/wave".equals(chatMessage)) {
> +                        if ("/wave".equals(chatMessage)) {
>                              SpaceEvent waveEvent = new SpaceEvent(
> SpaceEvent.PLAY_ANIMATION_EVENT);
>                              waveEvent.setProperty(SpaceEvent.USERNAME,
> user.getUsername());
>                              waveEvent.setProperty(SpaceEvent.ANIMATION_ID,
> new Long(3));
>                              listener.generatedSpaceEvent(waveEvent,
> SpaceSimulator.this);
>                              continue;
> -                        } else if("/point".equals(chatMessage)) {
> +                        } else if ("/point".equals(chatMessage)) {
>                              SpaceEvent waveEvent = new SpaceEvent(
> SpaceEvent.PLAY_ANIMATION_EVENT);
>                              waveEvent.setProperty(SpaceEvent.USERNAME,
> user.getUsername());
>                              waveEvent.setProperty(SpaceEvent.ANIMATION_ID,
> new Long(4));
>                              listener.generatedSpaceEvent(waveEvent,
> SpaceSimulator.this);
>                              continue;
>                          }
> -
> +
>                          chatMessage = markdownChatMessage(chatMessage);
>
>                          SpaceEvent markedUpEvent = new SpaceEvent(
> SpaceEvent.TEXT_SAY_EVENT);
> @@ -405,7 +413,7 @@
>
>                      } else if (SpaceEvent.MESSAGE_BROWSER_EVENT.equals(
> event.getName())) {
>                          listener.generatedSpaceEvent(event,
> SpaceSimulator.this);
> -
> +
>                      } else {
>                          System.err.println("Received a space event " +
> event.getName() + ", and dropping it on the floor.");
>                      }
> @@ -790,29 +798,29 @@
>      }
>
>      public long getVacancyTime() {
> -        if(getUserCount() > 0) {
> +        if (getUserCount() > 0) {
>              return 0;
>          }
>          return System.currentTimeMillis() - lastLogoutTime;
>      }
> -
> +
>      private class UserCounts {
>          private HashMap map = new HashMap();
>
>          public UserCounts() {
>          }
> -
> +
>          synchronized int getTotalUserCount() {
> -            String[] keys = (String[])map.keySet().toArray(new
> String[0]);
> +            String[] keys = (String[]) map.keySet().toArray(new
> String[0]);
>              int result = 0;
>              for (int i = 0; i < keys.length; i++) {
> -                result += ((Integer)map.get(keys[i])).intValue();
> +                result += ((Integer) map.get(keys[i])).intValue();
>              }
>              return result;
>          }
> -
> +
>          synchronized void bootUser(String username) {
> -            while(decrementUserCount(username) > 0) {
> +            while (decrementUserCount(username) > 0) {
>                  //do nothing
>              }
>          }
> @@ -847,7 +855,7 @@
>                  map.remove(username);
>                  User user = space.getUser(username);
>                  if (user == null) {
> -                    System.err.println("Tried to remove a user that
> wasn't there: " + username);
> +                    System.out.println("Tried to remove a user that
> wasn't there: " + username);
>                      return 0;
>                  }
>
> @@ -876,7 +884,7 @@
>      }
>
>      private static final SimpleDateFormat LOG_DATE_FORMAT = new
> SimpleDateFormat("yyyy.MM.dd HH:mm:ss z");
> -
> +
>      public void log(String message) {
>          if (log.size() > 400) {
>              log.remove(0);
> @@ -895,14 +903,14 @@
>          for (int i = 0; i < thingDocs.length; i++) {
>              spaceDoc.addThingDocument(thingDocs[i]);
>          }
> -
> -        synchronized(settings) {
> -            String[] keys = (String[])settings.keySet().toArray(new
> String[0]);
> +
> +        synchronized (settings) {
> +            String[] keys = (String[]) settings.keySet().toArray(new
> String[0]);
>              for (int i = 0; i < keys.length; i++) {
> -                spaceDoc.addSetting(keys[i],
> (String)settings.get(keys[i]));
> +                spaceDoc.addSetting(keys[i], (String) settings.get
> (keys[i]));
>              }
>          }
> -
> +
>          DoorDocument[] doorDocs = getDoorDocuments();
>          for (int i = 0; i < doorDocs.length; i++) {
>              spaceDoc.addDoorDocument(doorDocs[i]);
> @@ -915,6 +923,14 @@
>          return space;
>      }
>
> +    public void setDeleted() {
> +        this.deleted = true;
> +    }
> +
> +    public boolean getDeleted() {
> +        return this.deleted;
> +    }
> +
>      public ThingDocument thingUpdated(ThingDocument proposedDoc) {
>          ArgumentUtils.assertNotNull(proposedDoc);
>          Thing thing = space.getThing(proposedDoc.getThingID());
> @@ -957,34 +973,34 @@
>      }
>
>      public Map getSettings() {
> -        synchronized(settings) {
> -            return (Map)settings.clone();
> +        synchronized (settings) {
> +            return (Map) settings.clone();
>          }
>      }
>
> -    public String getSetting(String key) {
> -        if(key == null) {
> +    public String getSetting(String key) {
> +        if (key == null) {
>              return null;
>          }
> -        synchronized(settings) {
> -            return (String)settings.get(key);
> +        synchronized (settings) {
> +            return (String) settings.get(key);
>          }
>      }
> -
> +
>      public void putSetting(String key, String value) {
>          ArgumentUtils.assertNotEmpty(key);
>          ArgumentUtils.assertNotEmpty(value);
> -        synchronized(settings) {
> +        synchronized (settings) {
>              settings.put(key, value);
>          }
>      }
> -
> +
>      public String removeSetting(String key) {
> -        synchronized(settings) {
> -            return (String)settings.remove(key);
> +        synchronized (settings) {
> +            return (String) settings.remove(key);
>          }
>      }
> -
> +
>      private class InSimErrorGeometryProvider implements GeometryProvider
> {
>
>          public InputStream getAnimationStream(long animationID) throws
> IOException {
>
> Modified: spaces/trunk/src/com/ogoglio/sim/site/SimServlet.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/sim/site/SimServlet.java       2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/sim/site/SimServlet.java       2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -203,6 +203,17 @@
>                  return;
>              }
>          }
> +
> +        public void doDelete(HttpServletRequest request,
> HttpServletResponse response, String[] pathElementsauthedAccount) throws
> ServletException, IOException {
> +          long spaceID = Long.parseLong(pathElementsauthedAccount[
> pathElementsauthedAccount.length-1]);
> +
> +          SpaceSimulator spaceSim = sim.getOrCreateSpaceSimulator
> (spaceID);
> +          spaceSim.setDeleted();
> +          sim.shutdownSpaceSim(spaceID);
> +          response.setStatus(HttpServletResponse.SC_OK);
> +          response.setContentLength(0);
> +          return;
> +      }
>      }
>
>      private class SettingsResource extends SiteResource {
>
> Modified: spaces/trunk/src/com/ogoglio/site/AccountServlet.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/site/AccountServlet.java       2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/site/AccountServlet.java       2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -676,9 +676,8 @@
>              }
>
>              String displayName = spaceElement.getStringAttribute(
> SpaceDocument.DISPLAY_NAME);
> -            String ownerUsername = spaceElement.getStringAttribute(
> SpaceDocument.OWNER_USERNAME);
>
> -            SpaceRecord newSpace = SpacePersistTasks.createSpace(displayName,
> ownerUsername, getSessionFactory());
> +            SpaceRecord newSpace = SpacePersistTasks.createSpace(displayName,
> requestedAccount.getUsername(), getSessionFactory());
>              if (newSpace == null) {
>                  response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
>                  return;
> @@ -1100,7 +1099,6 @@
>              long bodyID = Long.parseLong(pathElements[pathElements.length- 1]);
>              BodyRecord record = BodyPersistTasks.findBodyByID(bodyID,
> getSessionFactory());
>              if (record == null || !record.getOwnerUsername().equals(
> requestedAccount.getUsername())) {
> -                System.out.println("No such body: " + bodyID);
>                  response.setStatus(HttpServletResponse.SC_NOT_FOUND);
>                  return;
>              }
>
> Modified: spaces/trunk/src/com/ogoglio/site/SpaceServlet.java
> ===================================================================
> --- spaces/trunk/src/com/ogoglio/site/SpaceServlet.java 2007-07-10
> 18:13:02 UTC (rev 223)
> +++ spaces/trunk/src/com/ogoglio/site/SpaceServlet.java 2007-07-10
> 20:03:06 UTC (rev 224)
> @@ -16,6 +16,7 @@
> import java.io.IOException;
> import java.net.URI;
> import java.net.URISyntaxException;
> +import java.util.Date;
>
> import javax.servlet.ServletConfig;
> import javax.servlet.ServletException;
> @@ -26,6 +27,8 @@
>
> import com.ogoglio.client.SpaceClient;
> import com.ogoglio.client.WebAPIClient;
> +import com.ogoglio.client.WebAPIUtil;
> +import com.ogoglio.media.MediaService;
> import com.ogoglio.persist.AccountRecord;
> import com.ogoglio.persist.PersistException;
> import com.ogoglio.persist.SimPersistTasks;
> @@ -55,9 +58,9 @@
>              config.getServletContext().setAttribute(MESSAGE_PROXY_KEY,
> messageProxy);
>          } catch (IOException e) {
>              throw new ServletException("Could not start the message
> proxy: " + e);
> -        }
> +        }
>      }
> -
> +
>      public void destroy() {
>          try {
>              super.destroy();
> @@ -164,6 +167,38 @@
>              return;
>          }
>
> +        public void doDelete(HttpServletRequest request,
> HttpServletResponse response, String[] pathElements, AccountRecord
> authedAccount) throws PersistException, ServletException, IOException {
> +            long spaceID = Long.parseLong(pathElements[
> pathElements.length - 1]);
> +            SpaceRecord spaceRecord =
> SpacePersistTasks.findSpaceBySpaceID(spaceID, getSessionFactory());
> +            if (spaceRecord == null) {
> +                response.setStatus(HttpServletResponse.SC_NOT_FOUND);
> +                return;
> +            }
> +
> +            if (authedAccount == null ||
> !authedAccount.getUsername().equals(spaceRecord.getOwnerUsername())) {
> +                response.setStatus(HttpServletResponse.SC_FORBIDDEN);
> +                return;
> +            }
> +
> +            if (spaceRecord.getSimID() != -1) {
> +                SimRecord simRecord = SpacePersistTasks.findOrAssignSim(spaceRecord,
> getSessionFactory());
> +                URI spaceURI = WebAPIUtil.appendToURI(simRecord.getSimURI(),
> "space/" + spaceID);
> +                if (!WebAPIClient.sendDelete(spaceURI, null)) {
> +                    response.setStatus(
> HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
> +                    return;
> +                }
> +            }
> +
> +            // delete the space from database
> +            SpacePersistTasks.deleteSpace(spaceRecord,
> getSessionFactory());
> +
> +            // delete the file if exists
> +            getMediaService().delete(MediaService.getSpaceDocumentName
> (spaceID));
> +
> +            System.out.println(new Date() + ": Deleted space: " +
> spaceID);
> +
> +            response.setStatus(HttpServletResponse.SC_OK);
> +        }
>      }
>
>      private class DoorsResource extends DescendingSiteResource { //NOTE
> this will proxy eveything below "door" in the URL space
>
>
> This was sent by the SourceForge.net collaborative development platform,
> the world's largest Open Source development site.
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> Ogoglio-commits mailing list
> Ogo...@li...
> https://lists.sourceforge.net/lists/listinfo/ogoglio-commits
>

Hi Trevor,

Thanks for adding the patches... I still need to figure out what goes where
:) and how.

In regards to using log4j I believe it to be a better solution than using
System.out/err, since we can benefit from the logging levels, changing
logging level at runtime, code guards etc.

Just as an example, "System.out.println(new Date() + ": Deleted space: " +
spaceID);"
Why would I want to have this line in the logs when the system is stable? It
can help me debugging the system but not more than that. If we used log4j I
might have marked this message as info level and usually I would run the
system at warn level...

In regards to the space delete issue, I think it make more sense to put the
DELETE in the AccountServlet and not in the SpaceServlet. The reason is that
since spaces are created in the AccountServlet it would make more sense to
delete it in the same place i.e. AccountServlet. Also, since SpaceServlet
manages the space's inner data only, I think it is better that the creation
and deletion of space stays in the level of the account.

Gal.

Hey, Gal.

On 7/11/07, Gal Nitzan <gal...@gm...> wrote:
> Thanks for adding the patches... I still need to figure out what goes where
> :) and how.

Indeed, there are a lot of moving parts despite my best efforts to
pare it down to the basics.

> In regards to using log4j I believe it to be a better solution than using
> System.out/err, since we can benefit from the logging levels, changing
> logging level at runtime, code guards etc.

After using it for several projects I've found that log4j  is usually
overkill, code verbose, and misused.  The line between debug, warning,
and info aren't at all clear and in general it takes futzing to use
properly.  My tactic is to only check in println's for the information
we'd want in the server log for a production machine.   During debug
we just use a debugger or add and remove println's as they're needed.

In addition, log4j is another moving part with its own config file and
API.  See my first comment about trying to pare down to the basics.
:-)

> In regards to the space delete issue, I think it make more sense to put the
> DELETE in the AccountServlet and not in the SpaceServlet. The reason is that
> since spaces are created in the AccountServlet it would make more sense to
> delete it in the same place i.e. AccountServlet. Also, since SpaceServlet
> manages the space's inner data only, I think it is better that the creation
> and deletion of space stays in the level of the account.

You make a valid point about my questionable decision to separate POST
and DELETE for space creation and deletion.  After looking at the
options I think it makes more sense to move space creation over to
SpaceServlet (where deletion is now) and make the
/og/account/username/space/ resource be a read-only list of an
account's spaces.

I'll try to get to that today.

- Trevor

Hi Trevor,

> -----Original Message-----
> From: ogo...@li... [mailto:ogoglio-
> dev...@li...] On Behalf Of Trevor F. Smith
> Sent: Wednesday, July 11, 2007 8:20 PM
> To: ogo...@li...
> Subject: Re: [Ogoglio-developers] [Ogoglio-commits] SF.net SVN:
> ogoglio:[224] spaces/trunk/src/com/ogoglio
>
> Hey, Gal.
>
> On 7/11/07, Gal Nitzan <gal...@gm...> wrote:
> > Thanks for adding the patches... I still need to figure out what goes
> where
> > :) and how.
>
> Indeed, there are a lot of moving parts despite my best efforts to
> pare it down to the basics.
[Gal Nitzan]
Until everything is in its place :)


>
> > In regards to using log4j I believe it to be a better solution than
> using
> > System.out/err, since we can benefit from the logging levels, changing
> > logging level at runtime, code guards etc.
>
> After using it for several projects I've found that log4j  is usually
> overkill, code verbose, and misused.  The line between debug, warning,
> and info aren't at all clear and in general it takes futzing to use
> properly.  My tactic is to only check in println's for the information
> we'd want in the server log for a production machine.   During debug
> we just use a debugger or add and remove println's as they're needed.
>
> In addition, log4j is another moving part with its own config file and
> API.  See my first comment about trying to pare down to the basics.
> :-)
[Gal Nitzan]
Well I'm familiar with the debate logging vs. println :-) fine by me either 
way.

>
> > In regards to the space delete issue, I think it make more sense to put
> the
> > DELETE in the AccountServlet and not in the SpaceServlet. The reason is
> that
> > since spaces are created in the AccountServlet it would make more sense
> to
> > delete it in the same place i.e. AccountServlet. Also, since
> SpaceServlet
> > manages the space's inner data only, I think it is better that the
> creation
> > and deletion of space stays in the level of the account.
>
> You make a valid point about my questionable decision to separate POST
> and DELETE for space creation and deletion.  After looking at the
> options I think it makes more sense to move space creation over to
> SpaceServlet (where deletion is now) and make the
> /og/account/username/space/ resource be a read-only list of an
> account's spaces.
[Gal Nitzan]
Great, it is even a better choice than mine. A good problem domain separation 
:).

>
> I'll try to get to that today.
[Gal Nitzan]

Thanks,

Gal
>
> - Trevor
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by DB2 Express
> Download DB2 Express C - the FREE version of DB2 express and take
> control of your XML. No limits. Just data. Click to get it now.
> http://sourceforge.net/powerbar/db2/
> _______________________________________________
> Ogoglio-developers mailing list
> Ogo...@li...
> https://lists.sourceforge.net/lists/listinfo/ogoglio-developers